fix(meshcore): auto-retry DM via same-path + flood cadence on ack timeout

[Yeraze]

Summary

• Fixes [BUG] MeshCore: Sending DMs does not try to find a new path if necessary #3977 — MeshCore DMs sent on a stale cached path went out once and were never retried, so they silently never arrived.
• Implements the official MeshCore app's retry cadence (MeshCore FAQ §5.3): on ack timeout, resend on the current cached path up to DM_SAME_PATH_RETRIES (2) times; once those are exhausted, reset the cached path (same effect as the existing manual "Reset Path" button) and resend via flood up to DM_FLOOD_RETRIES (1) time; if still unacked, mark the message failed. That's 2 same-path + 1 flood = 3 retries, 4 transmissions total including the initial send.
• Single bubble, not a duplicate: an auto-retry resend reuses the original message row — it does not create a new message or re-emit a message event. Instead, each retry re-points the original message's tracked expectedAckCrc/estTimeout via a new meshcore:message:updated event, so the UI shows one message bubble that ends in delivered (any ACK) or failed (all retries exhausted) — never a duplicated/contradictory pair of bubbles.
• Channel/broadcast sends are unaffected (unacked by design, never register a retry).
• Pending retry timers are cleared on disconnect(), alongside the existing pendingCommands cleanup.
• Went through two rounds of automated PR review; all flagged issues (ackCrc-collision timer leak, reset_path running outside the serialized send queue, a redundant-but-cheap connected re-check, and test coverage for the reset_path failure path) were fixed and verified. See PR comments for details, including a documented analysis of one review suggestion that turned out to be a false positive on closer trace-through of the CRC hand-off between attempts.

Changes

• src/server/meshcoreManager.ts:
  • pendingDmRetries map keyed by the firmware's expectedAckCrc, carrying remaining same-path/flood retry budget per in-flight DM.
  • performScopedSend arms the initial retry timer for DM sends (not channel sends, not retry resends themselves — gated by isAutoRetry).
  • The send_confirmed bridge-event handler cancels the timer when the ack matches.
  • scheduleDmAckTimeout / handleDmAckTimeout drive the same-path → flood → fail cadence; updateDmAttempt / failDmDelivery re-point or terminate the tracked message via emitMeshCoreMessageUpdated.
  • disconnect() clears any pending retry timers.
• src/server/services/dataEventEmitter.ts: new emitMeshCoreMessageUpdated event.
• src/components/MeshCore/hooks/useMeshCore.ts: onMessageUpdated handler keeps a single message bubble in sync across retries, re-arming its own client-side fail-safe timer against each new CRC.
• src/server/meshcoreManager.dmRetry.test.ts: covers ack-before-timeout, same-path retries, flood fallback after same-path exhaustion, terminal failure after all retries, single-bubble behavior (no duplicate emits), reset_path failure, channel-send exclusion, and timer cleanup on disconnect.

Test plan

• npx vitest run src/server/meshcoreManager — 251 tests passing (existing + new).
• npx tsc --noEmit — clean.
• npx eslint src/server/meshcoreManager.ts — no new errors (pre-existing repo-wide findings confirmed present on main independent of this change).
• Full npx vitest run — only pre-existing failures unrelated to MeshCore (missing protobufs/meshtastic/*.proto asset in this sandbox, confirmed present on main before this branch).
• Not yet verified against real hardware exhibiting a broken path.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y9S9qVAj7jW912hpYscXJW

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(meshcore): auto-retry DM via flood on ack timeout

Tasks:

• Gather context
• Read changed files
• Review code quality and best practices
• Post final review

---

Overall Assessment

This is a well-scoped, thoughtfully designed "Phase 1" fix. The implementation is clean, the failure modes are handled correctly, and the commentary/documentation is exemplary. The deliberate choice not to track the retry's own ack is reasonable and clearly explained. A few items worth considering:

---

Potential Bugs / Issues

1. ackCrc collision — low probability but possible
(meshcoreManager.ts:2727)

pendingDmRetries is keyed by ackCrc (a 16-bit CRC per MeshCore conventions). If two DMs are sent in rapid succession and the firmware happens to assign the same CRC to both (birthday problem over 65 536 values), the second scheduleDmAckTimeout call silently overwrites the first entry in the map, leaking the first timer (it fires but pendingDmRetries.get(ackCrc) returns the second entry's data, so the first DM's text is lost and the second DM is double-retried).

// current
this.pendingDmRetries.set(ackCrc, { toPublicKey, text, timer });
// if an entry already exists under ackCrc, the old timer is orphaned

A minimal guard:

private scheduleDmAckTimeout(...): void {
  const existing = this.pendingDmRetries.get(ackCrc);
  if (existing) clearTimeout(existing.timer); // don't orphan the old timer
  const timer = setTimeout(...);
  this.pendingDmRetries.set(ackCrc, { toPublicKey, text, timer });
}

2. resetContactPath calls sendBridgeCommand outside runSerialized
(meshcoreManager.ts:2750–2754)

handleDmAckTimeout calls resetContactPath directly (which calls sendBridgeCommand('reset_path', ...)), then immediately calls runSerialized(() => performScopedSend(...)). The reset_path command isn't serialized, so it can interleave with a concurrent user-initiated send that's already in runSerialized. In practice this is unlikely to cause visible problems, but it breaks the invariant that all device I/O goes through the serialized queue. Wrapping the whole handler body in runSerialized would be safer:

await this.runSerialized(async () => {
  const reset = await this.resetContactPath(pending.toPublicKey);
  if (!reset) return;
  await this.performScopedSend(pending.text, pending.toPublicKey, undefined, undefined, true);
});

(This also eliminates the nested runSerialized inside performScopedSend for the retry path, since performScopedSend is private and only called from inside runSerialized in all other paths.)

3. Missing connected re-check after resetContactPath
(meshcoreManager.ts:2744, 2750–2754)

The connected guard is checked at line 2744, but then there's an await for resetContactPath, which itself is an async bridge command. If the manager is torn down during that await (disconnect clears pendingDmRetries but doesn't cancel in-flight bridge commands), the subsequent performScopedSend will hit !this.connected and return { ok: false } — so no data loss, but an error is logged unnecessarily. A second guard after resetContactPath returns would be cleaner:

const reset = await this.resetContactPath(pending.toPublicKey);
if (!reset || !this.connected) return;

---

Code Quality & Best Practices

4. The isAutoRetry flag is a clean design choice — threading it through performScopedSend as a simple boolean avoids re-entry without any complex state. Well done.

5. The 20% margin (estTimeout * 1.2) is opaque
(meshcoreManager.ts:2726)

Consider hoisting it to a named constant alongside the other private static readonly class members near the top of the file, e.g.:

private static readonly DM_ACK_TIMEOUT_MARGIN = 1.2;

This makes it tunable from one place and documents the intent better than an inline literal.

6. void this.handleDmAckTimeout(ackCrc) — appropriate use of void for a floating promise from a setTimeout callback, consistent with repo style. No issue.

---

Test Coverage

The test suite is solid:

• ✅ Ack arrives before timeout → no retry
• ✅ No ack → reset + flood retry
• ✅ Flood retry also unacked → no second retry (no loop)
• ✅ Ack for flood retry arrives → no further action
• ✅ Channel send → no retry scheduled
• ✅ Disconnect → timers cleared

One gap: no test for the resetContactPath failure path (reset_path returns success: false). The code path if (!reset) return at line 2751 is untested. Given that makeManager already has a resetPathOk option, adding this would be trivial.

---

Performance

No concerns. The Map operations are O(1), timer overhead is negligible, and the retry fires at most once per DM. The Math.round(estTimeout * 1.2) call is a single arithmetic op in a rarely-executed code path.

---

Security

No concerns. The retry resends the original text and toPublicKey captured at send time — no injection risk from bridge event data. The !this.connected guard prevents a torn-down manager from interacting with a new connection's state.

---

Minor Nit

The test file comment block at lines 1–11 is unusually verbose for a test file header, but it's in keeping with the codebase's documentation style and accurately describes the behavior, so this is just a style observation.

---

Summary: The implementation is correct and production-ready for a Phase 1. Items 1 (timer orphan on CRC collision) and 2 (race between reset_path and serialized send queue) are worth addressing before merge — item 1 in particular because a CRC collision produces a subtle mis-retry rather than a visible error. Item 3 is a minor defensive hardening. The missing resetPathOk=false test is low-risk but easy to add.

[Yeraze]

Thanks for the review — addressed all three bug findings plus the readability/test-coverage suggestions in 5c8be4d:

1. ackCrc collision: scheduleDmAckTimeout now clears any existing timer for that CRC before overwriting the map entry, so a collision can't orphan a timer.
2. reset_path outside the serialized queue: the reset + flood resend now run together inside the same runSerialized call sendMessageWithResult uses, so they can't interleave with a concurrent user-initiated send's scope-assert→send pair.
3. Missing connected re-check: added after the resetContactPath await returns.
4. Hoisted the 20% margin to a DM_ACK_TIMEOUT_MARGIN constant.
5. Added the missing reset_path-failure test.

Full manager suite (248 tests) + typecheck + lint all clean. Still a draft — not merging.

-- Authored by Roger 🤓

---

Generated by Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(meshcore): auto-retry DM via flood on ack timeout

Tasks:

• Gather context
• Read changed files
• Review code quality and best practices
• Post final review

---

Overall Assessment

The previous review findings have all been addressed cleanly in 5c8be4d. The implementation is correct and production-ready for a Phase 1 auto-retry. Below is a fresh review of the current state.

---

Bug Fixes Verified

All three bugs from the previous review are correctly resolved:

1. ackCrc collision (timer orphan) — scheduleDmAckTimeout now correctly cancels the existing timer before overwriting the map entry (meshcoreManager.ts:2733-2734). The JSDoc comment also accurately explains the 16-bit CRC space and why the guard is needed.

2. reset_path outside serialized queue — handleDmAckTimeout now wraps the entire resetContactPath + performScopedSend pair inside a single runSerialized call (meshcoreManager.ts:2768-2772). This preserves the scope-assert→send atomicity invariant from #3667. The JSDoc on the method correctly explains why performScopedSend is called directly instead of nesting another runSerialized.

3. Missing connected re-check after resetContactPath await — The guard if (!reset || !this.connected) return at line 2770 correctly short-circuits both cases: reset_path failure AND a disconnect that occurred while the bridge command was in-flight.

---

Code Quality

Implementation is clean and well-documented.

• DM_ACK_TIMEOUT_MARGIN = 1.2 is properly hoisted to a named constant (meshcoreManager.ts:2721).
• The isAutoRetry boolean threading is minimal and correct — the retry's own performScopedSend call won't schedule a second timer.
• disconnect() clears pending timers before setting this.connected = false (meshcoreManager.ts:1067-1072), which means the !this.connected guard inside handleDmAckTimeout (line 2762) is a belt-and-suspenders guard for the race window between timer fire and the disconnect path running. That's correct defensive programming.
• The send_confirmed handler correctly cancels the timer before emitting the event (meshcoreManager.ts:1357-1360).

One minor observation: the handleDmAckTimeout guard at line 2762 checks !this.connected before entering runSerialized. The connected state isn't protected by the serialized queue's lock, so a disconnect arriving between the guard and the runSerialized body executing is technically possible. However, since performScopedSend already returns { ok: false } when !this.connected (it checks internally), and resetContactPath also guards on connected, this race has no observable adverse effect — it just does slightly more work before failing gracefully. No action needed, just noting it.

---

Test Coverage

Test suite is complete. The new resetPathOk=false test (line 124-133) correctly covers the missing failure path from the previous review. All six scenarios are present:

• ✅ Ack before timeout → no retry
• ✅ No ack → reset + flood retry
• ✅ Flood retry also unacked → no second retry
• ✅ reset_path fails → resend skipped
• ✅ Ack for flood retry cancels its timer
• ✅ Channel sends → no retry scheduled
• ✅ Disconnect clears timers

The makeManager helper's sendIdx capping (Math.min(sendIdx, sends.length - 1)) means the second send in a retry test reuses the last configured ackCrc/estTimeout entry — this correctly simulates the retry getting its own ackCrc (222) for the "cancels the retry if the ack for the flood resend arrives" test.

---

Performance & Security

No concerns. The Map operations are O(1), the retry fires at most once per DM, and the text/publicKey captured at send time carry no injection risk.

---

Summary

The implementation is correct, all previous findings addressed, test coverage is complete, and the code is well-documented. Ready to merge when the author is satisfied with hardware validation.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(meshcore): auto-retry DM via flood on ack timeout

Tasks:

• Gather context
• Read changed files
• Review code quality and best practices
• Post final review

---

Overall Assessment

This is the third review pass against this PR. The two prior rounds addressed ackCrc collision, serialization ordering, connected-guard placement, the opaque margin constant, and the missing reset_path failure test. All of those are cleanly resolved. The current revision is a significant expansion — it now implements the full official MeshCore retry cadence (same-path retries → flood → fail), including a new meshcore:message:updated event pipeline that threads through dataEventEmitter, the WebSocket layer, and useMeshCore.ts to keep a single UI bubble alive across the entire retry sequence.

---

What's New vs the Previous Round

The first review covered a one-shot "flood on timeout" approach. The code has since been upgraded to match MeshCore FAQ §5.3 exactly:

• DM_SAME_PATH_RETRIES = 2 same-path resends before touching the cached path
• DM_FLOOD_RETRIES = 1 flood after exhausting same-path
• updateDmAttempt / failDmDelivery / emitMeshCoreMessageUpdated for single-bubble tracking
• onMessageUpdated in useMeshCore.ts re-arms the frontend fail timer to the new CRC on each retry

This is substantially more code than was originally described in the PR summary. The test file header and test names accurately reflect the new behavior, but the PR description itself still describes only the Phase 1 (one-shot flood) design. That's a documentation mismatch to be aware of when reading the PR body — the actual implementation is the correct fuller behavior, but a reviewer reading only the PR description would think less is here.

---

Bugs / Issues

1. failDmDelivery called with the last attempted CRC inside runSerialized, but the frontend timer was armed against a different (new) CRC

meshcoreManager.ts:2872

const result = await this.performScopedSend(..., true);
if (!result.ok || result.expectedAckCrc == null || result.estTimeout == null) {
  this.failDmDelivery(pending.messageId, ackCrc);  // ← uses outer ackCrc, not result.expectedAckCrc
  return;
}

When performScopedSend returns ok: true (the firmware accepted the send) but then expectedAckCrc is somehow null (unusual but possible if the bridge response omits the field), failDmDelivery emits previousAckCrc = ackCrc (the previous attempt's CRC). The frontend already cancelled that timer via the updateDmAttempt call on the previous timeout, and there is no new timer to cancel for the current attempt's CRC because updateDmAttempt was never called. The frontend's per-message fail timer (armed to the previous CRC's successor, which was never registered) quietly leaks. This is a narrow edge case for a non-conforming bridge response, but the fix is trivial — pass whatever CRC was assigned (even undefined) so the frontend can clean up:

// if a new CRC was assigned, cancel its timer too before failing
this.failDmDelivery(pending.messageId, result.expectedAckCrc ?? ackCrc);

2. Disconnect guard path in handleDmAckTimeout (line 2830) calls failDmDelivery but the message may already be failed from a prior retry's failDmDelivery

If the same message's retry sequence hits disconnect on two separate timer callbacks (e.g., a disconnect races with an already-queued setTimeout), failDmDelivery emits a second deliveryStatus: 'failed' event. setMessages in useMeshCore uses prev.map(m => m.id !== evt.id ? m : {...m, ...}) so the duplicate write is idempotent in the React state. In practice this can only happen if disconnect fires twice (which it currently guards against), but it's worth noting.

No action needed — the duplicate is harmless.

---

Code Quality

3. isAutoRetry flag in performScopedSend is correctly threaded

meshcoreManager.ts:2705–2723 — the isAutoRetry guard correctly suppresses message creation, event emission, and timer arming on the retry path. The comment (An auto-retry resend…) accurately explains the invariant. Clean.

4. updateDmAttempt correctly updates in-memory message CRC

meshcoreManager.ts:2903–2907 — msg.expectedAckCrc = newAckCrc keeps the in-memory snapshot consistent so a snapshot pull after reconnect reflects the current attempt. This subtle correctness property is documented in the JSDoc.

5. onMessageUpdated in useMeshCore.ts:609–634 handles the full lifecycle cleanly

Cancels the previous attempt's fail timer → patches the existing message in place → arms a new fail timer for the new CRC. The logic correctly skips re-arming on terminal statuses. This is well-implemented.

6. emitMeshCoreMessageUpdated in dataEventEmitter.ts:329–347

The new emitter method correctly injects sourceId into data so the socket forwarder can route it to the right room, and also sets event.sourceId for source filtering. Consistent with the existing patterns in the file.

---

Test Coverage

The test suite is comprehensive for the new full-cadence behavior:

• ✅ No retry when ack arrives before timeout
• ✅ Same-path retries before flood (two timeouts, no reset)
• ✅ Flood only after same-path retries exhausted
• ✅ All retries exhausted → deliveryStatus: 'failed'
• ✅ Single bubble (no duplicate emitMeshCoreMessage, CRC re-pointed via emitMeshCoreMessageUpdated)
• ✅ Ack for a later retry cancels the sequence
• ✅ reset_path failure → flood skipped, message failed
• ✅ Channel sends → no retry
• ✅ contact_path_updated post-flood → schedulePathRefresh called
• ✅ Disconnect mid-sequence → timers cleared

One small gap: the test at line 140 (marks the message failed after all retries exhausted) asserts sends(bridgeCalls).toHaveLength(4). The comment says "1 initial + 2 same-path + 1 flood = 4 total." This is correct if the flood send succeeds. There is no test asserting sends count when the flood send itself returns ok: false (as opposed to reset_path returning false). Given makeManager always returns success: true for send_message, this case is only reachable if the bridge becomes disconnected mid-flood, which is a harder-to-test race. Low priority, but noting for completeness.

---

Performance

No concerns. The Map is O(1), retry budget is finite and small (at most 3 extra sends per DM), and the flood path runs inside the existing serialized queue so it can't pile up. scheduleDmAckTimeout calls clearTimeout before overwriting, preventing timer leaks.

---

Security

No concerns. pending.text and pending.toPublicKey are captured at send time from validated user input, not from bridge event data. No injection vectors introduced.

---

PR Description vs Implementation Gap

The PR summary still describes the old Phase 1 ("reset path and resend once via flood"). The implementation is now the full official cadence (2 same-path + 1 flood). Consider updating the PR body to match — reviewers and future git-blame readers will thank you.

---

Summary: The implementation is correct and complete for the full MeshCore retry cadence. Item 1 (edge-case failDmDelivery with wrong CRC when performScopedSend returns ok: true but no expectedAckCrc) is low-risk but worth a one-liner fix. All previously raised issues are resolved. The PR description should be updated to match the actual (fuller) implementation. Ready to merge pending hardware validation and the minor PR description update.

[Yeraze]

Thanks for the third pass. I traced through finding #1 carefully before deciding whether to change it, and I don't think it's actually a bug — writing up the reasoning so it's on record rather than silently skipping it:

handleDmAckTimeout(ackCrc)'s ackCrc parameter is always exactly the CRC most recently communicated to the frontend as the "live" attempt for that message:

• For the first timeout, that's the CRC from the original emitMeshCoreMessage (armed via scheduleDmAckTimeout right after the initial send).
• For every subsequent timeout, that's the CRC from the most recent updateDmAttempt → emitMeshCoreMessageUpdated, because scheduleDmAckTimeout(result.expectedAckCrc, ...) is called with exactly the same value that gets passed to the frontend, and that's the value handleDmAckTimeout is later invoked with when that timer fires.

So at line ~2872, when performScopedSend returns ok:true but somehow expectedAckCrc == null, calling failDmDelivery(pending.messageId, ackCrc) with the outer ackCrc is already passing the exact CRC the frontend currently has an active timer for — there's nothing stale about it. The suggested fix (result.expectedAckCrc ?? ackCrc) is actually a no-op in that branch specifically, since it's only reached when result.expectedAckCrc is null — the ?? always resolves to ackCrc, the same value already there. (In the !result.ok sub-case, using a expectedAckCrc from a send that didn't even succeed would be wrong to re-arm anything against.)

Not applying this one — happy to be shown a concrete repro where it actually leaks a timer, but I couldn't construct one. Finding #2 needs no action per your own note. Everything else from all three rounds is applied and tested (251 passing).

Updated the PR description to reflect the fuller same-path+flood cadence now implemented, rather than the original one-shot-flood Phase 1.

-- Authored by Roger 🤓

---

Generated by Claude Code