fix: surface routing NAKs on remote admin and node requests by jamesarich · Pull Request #7749 · meshtastic/Meshtastic-Android

jamesarich · 2026-10-23T12:47:19Z

A request the target or the mesh answers with a routing NAK was reported as unreachable after a timeout, or never resolved at all. Remote admin to a node that refuses our key said "Could not reach node" after 30 s, and traceroute, telemetry, position, user info, neighbor info and metadata requests sat on "Requesting" with nothing after.

🐛 Bug Fixes

MeshPacket.routingNakOrNull() reads a routing NAK (request id, sender, error) off meshPacketFlow, the same stream RadioConfigViewModel already watches.
EnsureRemoteAdminSessionUseCase picks the metadata request id itself and ends as Rejected when a NAK answers it. A NAK from the target shows its reason, e.g. "The remote node does not authorize your admin key."; our own node's key failures do too; a NAK from anywhere else still shows "Could not reach node". A fast local NAK ends the wait when it lands; a multi-hop MAX_RETRANSMIT usually arrives after the 30 s deadline, so that case still reads as a timeout.
RequestNakWatcher watches each node request for five minutes and shows " from failed: ". requestPosition and requestUserInfo take a request id, defaulting to one the sender assigns.
completeDispatchedResponse takes the sender. An ACK completes an awaited send only when it comes from the destination, so a relay's implicit ACK no longer reports a remote begin_edit_settings or commit_edit_settings as accepted. Remote edit boundaries now wait for the target's ACK, so if that ACK is lost while a relay's is heard, the edit fails after 30 s with EditSettingsTransactionException even when the write landed.

🧹 Chores

Spotless reflowed the untouched val deferred block in EnsureRemoteAdminSessionUseCase.kt once the file was in the diff.

Testing Performed

RoutingNakTest: NAKs are read with request and sender; ACKs, unanswered packets and other ports are not NAKs.
EnsureRemoteAdminSessionUseCaseTest: a NAK on the request id returns Rejected; a NAK for another id and a plain ACK are ignored.
RequestNakWatcherTest: a NAK to the request shows the failure; other ids and ACKs show nothing; a failed send stops watching.
PacketHandlerImplTest: a relay's ACK leaves the wait open and the destination's NAK ends it as rejected.
NodeDetailViewModelTest: a refusal from the target shows its detail; a NAK from another node shows unreachable.
Not reproduced on a device pair.

Firmware still reports a refused remote set_* write as a plain ACK, because MeshModule discards the refusal when the write did not ask for a response. That needs a firmware change and is not in this PR.

Fixes #7732

Summary by CodeRabbit

New Features
- Failed node requests now provide clearer feedback, including the request type, node name, and failure reason.
- Remote-admin metadata refresh rejections now show more specific guidance.
Bug Fixes
- Acknowledgments from relay nodes no longer incorrectly complete requests intended for another node.
- Metadata refresh now recognizes routing rejections associated with its request.

coderabbitai · 2026-10-23T12:47:28Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto
Repository: meshtastic/Meshtastic-Android
Branch: fix/remote-admin-routing-nak
File: .coderabbit.yaml
Commit: 09ab2b9
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: 659793b7-36bc-43c1-8b60-a8db5055576e

📥 Commits

Reviewing files that changed from the base of the PR and between bacb891 and 09ab2b9.

📒 Files selected for processing: 31

.skills/compose-ui/strings-index.txt
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerTest.kt
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt
core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCase.kt
core/domain/src/commonMain/kotlin/org/meshtastic/core/domain/usecase/session/EnsureSessionResult.kt
core/domain/src/commonTest/kotlin/org/meshtastic/core/domain/usecase/session/EnsureRemoteAdminSessionUseCaseTest.kt
core/model/src/commonMain/kotlin/org/meshtastic/core/model/RoutingNak.kt
core/model/src/commonTest/kotlin/org/meshtastic/core/model/RoutingNakTest.kt
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/QueryController.kt
core/resources/src/commonMain/composeResources/values/strings.xml
core/service/src/commonMain/kotlin/org/meshtastic/core/service/QueryControllerImpl.kt
core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeCommandSender.kt
core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeRadioController.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/AdministrationSection.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActions.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/HandleNodeAction.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModel.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcher.kt
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/model/NodeDetailAction.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/CommonNodeRequestActionsTest.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/HandleNodeActionTest.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailViewModelTest.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RecordingSnackbarManager.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/RequestNakWatcherTest.kt
feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/detail/TestNakWatcher.kt
feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/detail/NodeDetailCompassLifecycleTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The change adds routing NAK decoding, passes request IDs through request APIs, and watches node requests for matching NAKs. Remote-admin session handling now returns and displays a rejection when a matching NAK arrives.

Changes: Routing NAK feedback

Layer / File(s)	Summary
Decode NAKs and validate response completion `core/model/...`, `core/repository/...`, `core/data/...`, `core/model/src/commonTest/...`, `core/data/src/commonTest/...`	The model extracts routing NAKs with a nonzero request ID and a non-`NONE` error. Dispatched response completion receives the sender and leaves a unicast wait open for a positive response from a relay.
Pass request IDs through request APIs `core/repository/...`, `core/service/...`, `core/data/...`, `core/testing/...`	Metadata, position, and user-info request APIs accept request IDs and forward them to packet creation. Position and user-info packet creation generates a nonzero ID when the supplied value is zero.
Watch node requests and show matching NAKs `.skills/compose-ui/strings-index.txt`, `core/resources/...`, `feature/node/src/commonMain/...`, `feature/node/src/commonTest/...`, `feature/node/src/jvmTest/...`	Node request actions use `RequestNakWatcher` to assign IDs and observe matching NAKs. The watcher displays failure text for a matching NAK and cancels observation if sending fails. Metadata refresh passes the node name to the watcher.
Return and display remote-admin rejections `core/domain/...`, `feature/node/src/commonMain/...`, `feature/node/src/commonTest/...`	The remote-admin session flow matches NAKs to the metadata request and returns `EnsureSessionResult.Rejected`. Node detail handling displays routing-error text for applicable rejections and an unreachable-node message for NAKs from another node.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 09ab2

No actionable merge-blocking issue is established; this is ready for normal checks and merge.

Pre-merge checks: 6 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Tests Prove The Path, Not The End State		The filtering test remains a false positive. In `EnsureRemoteAdminSessionUseCaseTest.kt:172-191`, the test emits a wrong-request NAK and a `NONE` NAK, then emits the normal refresh and asserts only `E…	Change `ignores a nak for another request and a plain ack` to emit a wrong-request NAK and a plain ACK, followed by a matching NAK. Assert `EnsureSessionResult.Rejected` contains the matching request ID and error. Verify that metadata dispa…
Regression Coverage For Changed Behavior		Regression coverage remains incomplete in the current head: - `CommonNodeRequestActions` routes five requests through `RequestNakWatcher` (CommonNodeRequestActions.kt:84-145). Tests cover selected fai…	Add tests that (1) exercise every `CommonNodeRequestActions` request with a fixed watcher ID, verify each controller argument, and capture position and user-info packets to assert their IDs; (2) call `NodeDetailViewModel.refreshMetadata` wi…

Passed checks: 6 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: surfacing routing NAK errors for remote-admin and node requests.
Linked Issues check		Directly linked issue `#7732` asks that remote-admin refusals show the actual error, not "Could not reach node". The reviewed head has no changes since the previous review (currentHead equals previousRe…
Out of Scope Changes check		The reviewed head has no incremental changes since the previous review. The prior conclusion is preserved. Each change group ties to the NAK-surfacing objective stated for this PR and to `#7732`: - `Ro…
Sibling Call Sites And Presence Semantics		PASS. The diff changes request-ID and sender presence handling, not a physical metric field. No RSSI, temperature, current, voltage, particulate, humidity, or SNR field changed. The new `request_id !=…
Moved Code Diffed Against Its Original		The check is not triggered. The authoritative PR diff has no deleted files or deleted type/function declarations, and Git does not detect a rename or copy. The added `RoutingNak` and `RequestNakWatche…

Full details: Linked Issues check

Explanation

Directly linked issue #7732 asks that remote-admin refusals show the actual error, not "Could not reach node". The reviewed head has no changes since the previous review (currentHead equals previousReviewedHead), so the prior conclusion is preserved. A focused read of the head confirms the mechanism:

core/model/.../RoutingNak.kt: MeshPacket.routingNakOrNull() returns a RoutingNak(requestId, from, routingError) for a ROUTING_APP packet with nonzero request ID and a non-NONE error. It returns null for ACKs and other packets.
core/domain/.../EnsureRemoteAdminSessionUseCase.kt: runEnsure generates the request ID, subscribes to meshPacketFlow NAKs with that ID, passes the ID to refreshMetadata, and returns EnsureSessionResult.Rejected(nak) in the select. Timeout still returns Timeout after UX_TIMEOUT (30 s).
feature/node/.../NodeDetailViewModel.kt (per the change summary, not re-read): a NAK from the destination, or a local public-key error, shows the routing-error text. A NAK from any other node shows the unreachable text.
NodeDetailViewModelTest covers the target-refusal case and the other-node case.

The unknown-public-key case is handled the same way. A target NAK uses the routing-error text for that error. I did not confirm that every unknown-key error code has a specific string. Known limit, disclosed by the PR: a multi-hop MAX_RETRANSMIT NAK that arrives after 30 s still shows the timeout text. The issue does not state hop count, so this does not fail the requirement. Tests were not run in this review. The PASS rests on source reads and test descriptions.

Full details: Out of Scope Changes check

Explanation

The reviewed head has no incremental changes since the previous review. The prior conclusion is preserved. Each change group ties to the NAK-surfacing objective stated for this PR and to #7732:

RoutingNak, routingNakOrNull(), and RoutingNakTest provide the parsing layer for the remote-admin fix.
EnsureRemoteAdminSessionUseCase, EnsureSessionResult.Rejected, and their tests implement the remote-admin rejection path.
NodeDetailViewModel, CommonNodeRequestActions, HandleNodeAction, NodeDetailAction, and AdministrationSection pass the node name and watcher into the new failure message.
RequestNakWatcher and its tests extend NAK display to position, user info, telemetry, traceroute, and neighbor info requests. The PR objectives name these requests. The issue text covers only remote admin, so this extension is a judgment call. It uses the same mechanism and does not change unrelated behavior.
PacketHandlerImpl.completeDispatchedResponse(..., from) and MeshDataHandlerImpl stop a relay ACK from completing an awaited admin send. The PR objectives list this as in scope. It changes remote edit-settings timing. The PR discloses that a lost target ACK now causes a 30 s failure.
QueryController, CommandSender, CommandSenderImpl, QueryControllerImpl, and the fake classes add requestId parameters. These support the watcher and the metadata request ID.
strings.xml and .skills/compose-ui/strings-index.txt add the request_failed_from string used by the watcher.
The Spotless reflow in EnsureRemoteAdminSessionUseCase.kt touches a file that the fix changes.

No change was found that has no connection to the stated objective.

Full details: Sibling Call Sites And Presence Semantics

Explanation

PASS. The diff changes request-ID and sender presence handling, not a physical metric field. No RSSI, temperature, current, voltage, particulate, humidity, or SNR field changed. The new request_id != 0 check is consumed by both EnsureRemoteAdminSessionUseCase and RequestNakWatcher; changed request APIs forward the supplied ID through QueryControllerImpl, CommandSenderImpl, and fakes. The new 0 defaults represent an unspecified request ID, not a physical measurement.

Full details: Moved Code Diffed Against Its Original

Explanation

The check is not triggered. The authoritative PR diff has no deleted files or deleted type/function declarations, and Git does not detect a rename or copy. The added RoutingNak and RequestNakWatcher files contain new logic; the existing request methods remain in their original files with signature changes. The EnsureRemoteAdminSessionUseCase change only reformats the existing deferred block and adds NAK handling. No removed declaration has callers that require moved-code compatibility review.

Full details: Tests Prove The Path, Not The End State

Explanation

The filtering test remains a false positive. In EnsureRemoteAdminSessionUseCaseTest.kt:172-191, the test emits a wrong-request NAK and a NONE NAK, then emits the normal refresh and asserts only EnsureSessionResult.Refreshed. The base implementation at EnsureRemoteAdminSessionUseCase.kt:110-137 already dispatches metadata and returns Refreshed from that refresh, so the test still passes without the routing-NAK listener or request-ID filtering. The other added tests assert parsed values, a pending send, snackbar text, or watcher resolution; no collection-size-only or unstable Unconfined emission-order test was found.

Resolution

Change ignores a nak for another request and a plain ack to emit a wrong-request NAK and a plain ACK, followed by a matching NAK. Assert EnsureSessionResult.Rejected contains the matching request ID and error. Verify that metadata dispatch uses REQUEST_ID. This must fail when the NAK listener is reverted and when the wrong request is accepted.

Full details: Regression Coverage For Changed Behavior

Explanation

Regression coverage remains incomplete in the current head: - CommonNodeRequestActions routes five requests through RequestNakWatcher (CommonNodeRequestActions.kt:84-145). Tests cover selected failures and traceroute, but no successful position or telemetry wiring. CommandSenderImplTest checks position payload fields only with the default ID (lines 501-528), and has no successful user-info packet test. A wrong watcher ID would prevent NAK matching. - NodeDetailViewModel.refreshMetadata passes the watcher ID to QueryController (NodeDetailViewModel.kt:232-240). Its only test covers queue rejection and verifies the old default-argument call (NodeDetailViewModelTest.kt:217-226). No test verifies ID forwarding or a matching metadata NAK snackbar. - remoteAdminRefusalText adds local-PKI and generic-detail branches (NodeDetailViewModel.kt:71-79). Tests cover only a target ADMIN_PUBLIC_KEY_UNAUTHORIZED detail and a foreign MAX_RETRANSMIT. No test covers local PKI errors or a target error without a detail resource. openRemoteShell shares the rejected-result handling but has no rejection test. - RequestNakWatcher.watch starts collection before sending and expires after WINDOW (RequestNakWatcher.kt:46-68). Tests emit NAKs only after watch returns and never advance beyond WINDOW. They do not protect synchronous send/collection ordering or late-NAK suppression. - PacketHandlerImpl.completeDispatchedResponse now accepts ACKs from a destination or broadcast target, rejects relay ACKs, and accepts NAKs from any sender (PacketHandlerImpl.kt:474-480). The added test covers relay ACK followed by destination NAK, but not broadcast ACK acceptance or a NAK from a non-destination sender ending the wait.

Resolution

Add tests that (1) exercise every CommonNodeRequestActions request with a fixed watcher ID, verify each controller argument, and capture position and user-info packets to assert their IDs; (2) call NodeDetailViewModel.refreshMetadata with a fixed watcher ID and assert forwarding plus matching-NAK snackbar behavior; (3) cover local-PKI and target-generic remote-admin text and rejected openRemoteShell; (4) emit a matching NAK inside the send callback and assert no snackbar after advancing past RequestNakWatcher.WINDOW; and (5) verify broadcast ACK acceptance and foreign-sender NAK rejection completion in PacketHandlerImplTest.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

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 "<request> from <node> failed: <reason>" 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

jamesarich · 2026-10-23T18:56:09Z

@coderabbitai full review

coderabbitai · 2026-10-23T19:03:24Z

✅ Action performed

Full review finished.

github-actions Bot added the bugfix PR tag label Oct 23, 2026

jamesarich added 2 commits October 23, 2026 10:52

chore(resources): sort and index request_failed_from

09ab2b9

jamesarich force-pushed the fix/remote-admin-routing-nak branch from 1a01f2d to 09ab2b9 Compare October 23, 2026 15:53

jamesarich mentioned this pull request Oct 23, 2026

Answer refused requests and rate-limited phone sends with a NAK meshtastic/firmware#12225

Open

1 of 8 tasks

jamesarich marked this pull request as ready for review October 23, 2026 18:55

jamesarich enabled auto-merge October 23, 2026 19:03

jamesarich added this pull request to the merge queue Oct 23, 2026

Merged via the queue into main with commit dbaeb76 Oct 23, 2026
14 checks passed

jamesarich deleted the fix/remote-admin-routing-nak branch October 23, 2026 19:32

Navigation Menu

Repository navigation

fix: surface routing NAKs on remote admin and node requests - #7749

fix: surface routing NAKs on remote admin and node requests#7749
jamesarich merged 2 commits into
mainfrom
fix/remote-admin-routing-nak

jamesarich commented Oct 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

jamesarich commented Oct 23, 2026

Uh oh!

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jamesarich commented Oct 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🐛 Bug Fixes

🧹 Chores

Testing Performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesarich commented Oct 23, 2026

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jamesarich commented Oct 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading