fix(messaging): wait for shared-contact routing ACK

[simulationstation]

Summary

Bug fixes

• Keep transport queue admission separate from the routing ACK/NAK awaited by sendAdminAwait.
• Prevent an ordinary QueueStatus(res = 0) from releasing the first PKI DM before the local add_contact packet is processed.
• Correlate zero-ID queue-status fallbacks by packet ID instead of completing an unrelated pending routing response.
• Propagate routing NAKs, queue rejection, disconnected state, and transport-send failure as failed awaited sends.
• Fail a strict await immediately when disconnected instead of retaining its packet for transmission after reconnect.
• When a routing NAK arrives before QueueStatus, fail both deferred phases so the packet-queue worker is released.
• Cover queue admission, routing ACK/NAK, and immediate transport failure with regression tests.

Root cause

This is a residual sequencing path from #4988 and a follow-up to #4992 and #5005.

#4992 made shared-contact installation suspending, but sendToRadioAndAwait used the same deferred that the packet-queue worker uses for QueueStatus. QueueStatus(res = 0) reports that firmware accepted or queued the packet; it is not the later routing acknowledgement. On firmware 2.7.20, a self-addressed packet is queued for local processing and QueueStatus is returned before AdminModule runs. That allowed SendMessageUseCase to enqueue the dependent PKI DM before add_contact reached the firmware NodeDB.

The fix tracks those two phases separately. QueueStatus continues to release the transport queue worker, while the strict caller remains suspended until the correlated routing ACK/NAK arrives. A queue or local transport failure still fails both phases, and the existing synchronous ERRNO_SHOULD_RELEASE success remains supported.

This intentionally changes two shared-path behaviors: sendToRadioAndAwait now returns failure immediately while disconnected rather than leaving the packet queued for post-reconnect transmission, and a routing NAK received before its QueueStatus also fails the queue deferred so queue processing cannot remain blocked.

Firmware registers RoutingModule last, after AdminModule; for a valid shared contact, NodeDB::addFromContact completes before the routing ACK is generated. This makes that ACK a processing barrier for this path without changing the wire protocol.

Relevant protocol and firmware paths:

• QueueStatus schema: https://github.com/meshtastic/protobufs/blob/master/meshtastic/mesh.proto#L2309-L2320
• Local packet enqueue: https://github.com/meshtastic/firmware/blob/v2.7.20.6658ec2/src/mesh/Router.cpp#L239-L248
• add_contact handling: https://github.com/meshtastic/firmware/blob/v2.7.20.6658ec2/src/modules/AdminModule.cpp#L355-L363
• RoutingModule registration order: https://github.com/meshtastic/firmware/blob/v2.7.20.6658ec2/src/modules/Modules.cpp#L252-L254
• NodeDB mutation and save: https://github.com/meshtastic/firmware/blob/v2.7.20.6658ec2/src/mesh/NodeDB.cpp#L1756-L1814

Related work

• Addresses the residual ordering race reported in [Bug]: Message delivery Status: Unrecognized (was "Encrypted send failed"?) #4988.
• Follow-up to refactor(service): harden KMP service layer — database init, connection reliability, handler decomposition #4992, fix: improve PKI message routing and resolve database migration racecondition #4996, and fix: resolve correct node public key in sendSharedContact and favoriteNode #5005.
• fix(lifecycle): harden packet admission and transport ownership #6598 overlaps the packet-admission files, but its current implementation still completes awaited sends on ordinary QueueStatus; this PR separates the routing phase.

Testing Performed

• :core:data:testAndroidHostTest — 464/464 after the review follow-up.
• :core:repository:testAndroidHostTest — 21/21.
• Final focused PacketHandlerImplTest — 15/15, including zero-ID response correlation.
• :core:data:spotlessCheck :core:repository:spotlessCheck
• :core:data:detekt :core:repository:detekt
• kmpSmokeCompile

Limitations

A missing routing ACK is not proof that firmware failed to install the contact, so the existing caller remains best-effort after a NAK or timeout and still enqueues the DM. This PR fixes the deterministic queue-admission ordering race; it does not change message failure UX or retry policy.

[coderabbitai]

📝 Walkthrough

Walkthrough

sendToRadioAndAwait now waits for routing acknowledgements instead of queue acceptance. Transport and routing responses have separate tracking and cleanup paths. ACK, NAK, timeout, disconnection, queue rejection, and send-error behavior are covered by tests and documented.

Changes

Strict routing acknowledgement await

Layer / File(s)	Summary
Send-await lifecycle core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt, core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/*.kt	Strict-await sends track routing responses separately, reject disconnected radios, register transport responses before transmission, and clean up both response types. API documentation defines the new return semantics.
Routing completion and NAK state core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt, core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshDataHandlerImpl.kt	Queue-status handling and routing acknowledgement handling now complete responses according to ACK, NAK, failure, and synchronous loopback outcomes.
Behavior validation core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/*Test.kt	Tests cover disconnected sends, transport errors, queue acceptance, release statuses, routing ACKs, routing NAKs, and incomplete NAK response removal.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant PacketHandlerImpl
  participant Radio
  participant MeshDataHandlerImpl

  Caller->>PacketHandlerImpl: sendToRadioAndAwait(packet)
  PacketHandlerImpl->>Radio: enqueue and send packet
  Radio-->>PacketHandlerImpl: queue status
  PacketHandlerImpl-->>Caller: continue waiting for routing result
  Radio-->>MeshDataHandlerImpl: routing ACK or NAK
  MeshDataHandlerImpl->>PacketHandlerImpl: complete response
  PacketHandlerImpl-->>Caller: true for ACK, false for NAK

Loading

Possibly related PRs

• meshtastic/Meshtastic-Android#6598: Both changes update packet response handling and awaited-send behavior for routing ACK and NAK outcomes.

Suggested reviewers: jamesarich

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Tests Prove The Path, Not The End State	⚠️ Warning	Several changed tests do not prove the new path: the routing-ACK test manually calls removeResponse and would pass with the parent code; transport-failure and renamed queue tests also pass after re...	Drive tests through an actual routing ACK/NAK and verify the correlated request/side effect; retain assertions that distinguish routing completion from QueueStatus and reverted behavior.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sibling Call Sites And Presence Semantics	✅ Passed	PASS: The PR delta only changes packet/routing response handling and related tests/docs; it does not change metric nullability, zero guards, presence checks, or add physical metric fields.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: waiting for shared-contact routing acknowledgments.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[simulationstation]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/PacketHandlerImpl.kt`:
- Around line 192-196: Update the requestId == 0 fallback in PacketHandlerImpl
so it selects the first incomplete queue response, completes it, and uses that
entry’s packet ID to find and complete the corresponding routing response when
required. If no queue response exists, leave routingResponse untouched rather
than completing an unrelated pending entry.

In
`@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/PacketHandlerImplTest.kt`:
- Around line 205-217: The test strict await succeeds on routing ack must verify
it remains incomplete after queue acceptance. In strict await succeeds on
routing ack, add an assertFalse(result.isCompleted) immediately after
handleQueueStatus and testScheduler.runCurrent(), before
handler.removeResponse(794, complete = true), then retain the final success
assertion.

In
`@core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt`:
- Around line 59-64: Update the sendAdminAwait documentation in
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt:59-64
and sendToRadioAndAwait documentation in
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt:32-38
to document every strict-await outcome: return true for routing ACKs and
synchronous local-loopback delivery via ERRNO_SHOULD_RELEASE, and return false
for disconnection, transport-send failure, routing NAKs, queue rejection, and
timeout.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e76ac883-b5b7-4f65-b5f3-4df48ce87855

📥 Commits

Reviewing files that changed from the base of the PR and between a885127 and cfc59b2.

📒 Files selected for processing (6)

• 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/repository/src/commonMain/kotlin/org/meshtastic/core/repository/CommandSender.kt
• core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/PacketHandler.kt

[jamesarich]

Nice fix — the queue/routing phase split is the right mechanism, and the test matrix (queue-acceptance-doesn't-complete, ACK, NAK, disconnected, transport-throw, zero-ID) is exactly what this path needs. Merging as-is; two asks for the record and one follow-up:

1. Please add the two semantic changes to the description: (a) sendToRadioAndAwait now fails fast when disconnected instead of leaving the packet queued for post-reconnect transmission; (b) a routing NAK arriving before its QueueStatus now releases the queue deferred as failure. Both look right, but they're behavior changes on a shared path and should be discoverable from the PR record.

2. Resolving the CodeRabbit threads as non-blocking, with rationale: the zero-ID fallback correlation issue (queue and routing responses picked independently, potentially from different packets) is real but confined to the rare requestId == 0 fallback, and the strict routing path currently has a single production caller — the pre-existing ambiguity there predates this PR. Worth a follow-up fix (correlate both maps through the same selected entry), as is the suggested assertFalse(result.isCompleted) mid-test assertion and the KDoc on strict-await outcomes.

3. Heads-up: #6598 rewrites these same admission seams and will rebase over this — its typed-outcome machinery may eventually subsume the two-deferred split.