feat(meshcore): opt-in auto-retry for automated channel sends (#3979 Part 2)

[Yeraze]

Summary

Part 2 (final) of #3979 — an opt-in, one-shot auto-retry for automated MeshCore channel/broadcast sends when zero repeaters were heard within 30s. Builds directly on the now-merged Part 1 (PR #3987), which made the "heard repeaters" echo-attribution signal trustworthy.

A MeshCore channel send is an unacked, fire-and-forget flood — there is no firmware ACK. When Part 1's echo signal reports that no repeater re-flooded our packet within 30s, the message likely reached no one, so we resend it exactly once.

Behavior

• New global setting meshcoreChannelRetryEnabled, default OFF. Toggle lives in a new MeshCore Messaging section of global Settings (SettingsTab).
• Applies to automated senders only: Automation Engine action.sendMessage, Auto-Acknowledge, auto-responder, auto-announce, and timer triggers. Threaded via a new autoRetryOnMiss opt-in flag.
• User-initiated sends never retry — the meshcoreRoutes.ts send route does not opt in.
• One-shot: the resend is isAutoRetry=true (no new bubble, no message/data-bus emit — so it can't trigger a fresh automation) and autoRetryOnMiss=false (re-registered for echo correlation but never re-armed).
• Pending retries cleared in the disconnect() bulk cleanup.

Non-collision with the DM retry (#3977/#3980)

The DM ack-retry keys on the firmware ACK CRC (toPublicKey set); the channel retry keys on the echo-heard signal (channelIdx set). They are mutually exclusive branches of performScopedSend — a test asserts a DM send arms only pendingDmRetries and a channel send arms only pendingChannelRetries.

Heard-count read race

The 30s retry window equals the echo-attribution window (HEARD_WINDOW_MS), and correlateChannelEcho awaits its DB write before returning, so any repeater heard for a genuine echo is persisted before the timer reads it. A repeater heard after the read (extremely-late echo) yields at most one accepted duplicate — which the opt-in explicitly tolerates. A failed heard-count read aborts the retry (no needless duplicate).

Tests

New src/server/meshcoreManager.channelRetry.test.ts (8 tests, fake timers): zero-heard → one resend; heard → no resend; resend emits no message event / no bus re-entry; one-shot; user send never arms; setting-off never arms; disconnect clears; DM/channel non-collision. Full suite green (8181 passed, 0 failed); tsc clean.

Docs

docs/features/meshcore.md (new "Automated Channel-Send Auto-Retry" section) and a note in docs/features/automation-engine.md — opt-in, channel-only, one-shot, distinct from the DM retry.

Closes #3979

🤖 Generated with Claude Code

https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(meshcore): opt-in auto-retry for automated channel sends

Status: Review complete ✅

Tasks:

• Read PR context
• Review meshcoreManager.ts changes
• Review test file meshcoreManager.channelRetry.test.ts
• Review settings changes (SettingsTab, SettingsContext, constants)
• Review meshActionDeps.ts changes
• Compile and post review feedback

---

Overall Assessment

This is a clean, well-scoped implementation. The architecture is sound: fire-and-forget channel floods genuinely have no firmware ACK, so leaning on the Part 1 echo-attribution signal is the right approach. The one-shot, automated-only, opt-in constraints are all correctly enforced and clearly tested. Below are some observations ranging from minor to worthy of discussion.

---

Correctness / Logic

1. CHANNEL_RETRY_WINDOW_MS and HEARD_WINDOW_MS are both 30 s — is that a tight coupling risk?

meshcoreManager.ts:741,755 — Both constants are independently declared at 30 s. The PR description correctly points out they must be equal ("the retry window equals the echo-attribution window") so that the heard-repeater set is fully populated when the timer fires. However, they're separate constants with no enforced relationship. If someone increases HEARD_WINDOW_MS in the future to handle slower repeaters, they must also remember to update CHANNEL_RETRY_WINDOW_MS, or the retry will fire before the echo window closes — exactly the race the PR is designed to avoid.

Consider asserting the relationship explicitly, e.g. via a comment // MUST equal HEARD_WINDOW_MS — see §"Heard-count read race" in PR #3988 or a runtime/compile-time assertion. A simple approach would be a static initializer block or a test that asserts CHANNEL_RETRY_WINDOW_MS === HEARD_WINDOW_MS.

2. retriesLeft field is set but never decremented

meshcoreManager.ts:3123 — retriesLeft: 1 is stored in pendingChannelRetries, and line 3167 reads if (pending.retriesLeft <= 0) return; as a defensive guard. But retriesLeft is never decremented before the delete on line 3142 — so the guard on 3167 can never actually trip (the map entry is already deleted at 3142 before 3167 is reached). The "one-shot" guarantee is enforced correctly by autoRetryOnMiss=false on the resend (not by retriesLeft), so this is benign, but the field and its guard add misleading complexity. Either remove the field and the dead guard, or actually decrement and keep the entry for the check.

3. isChannelSend definition includes a channelIdx === undefined edge case

meshcoreManager.ts:2793 — const isChannelSend = !toPublicKey && channelIdx !== undefined; means a call with toPublicKey=undefined, channelIdx=undefined (the "broadcast on channel 0" case described in the JSDoc above) is classified as a DM send, not a channel send. Looking at the call site in handleChannelRetryTimeout (meshcoreManager.ts:3178-3185), the retry always passes the stored pending.channelIdx (guaranteed non-undefined since it was stored on a channel send), so the retry itself is fine. But the "both unset → broadcast on channel 0" case documented in the JSDoc is actually a DM-branch path, which seems like a pre-existing inconsistency worth a test.

---

Error Handling

4. No logging on the heardCount > 0 suppression path

meshcoreManager.ts:3161-3165 — The "heard by N repeaters → no retry" branch only logs at debug. This is fine for steady-state but can make it hard to diagnose "why didn't my message retry?" in production where debug is typically off. Consider elevating to info, consistent with the successful retry log at 3169.

5. Setting read failure on the maybeArmChannelRetry path aborts silently

meshcoreManager.ts:3108-3111 — A DB error reading meshcoreChannelRetryEnabled logs at warn and returns (no retry armed). This is the correct conservative choice (don't retry if you can't confirm intent), and it's noted in the PR description. But it's also invisible to the operator — they enabled the setting, sent an automated message, no retry fires, and there's no indication that a DB read failed. A brief note to the operator log (or surfacing via the existing notification mechanism) would be worthwhile for diagnosability.

---

Test Coverage

6. Tests are thorough for the happy paths — one missing scenario

The 8 tests cover: zero-heard → retry; heard → no retry; no bus re-entry; one-shot; user send; setting off; disconnect; DM/channel non-collision. Well done.

Missing: setting read throws → no retry armed. This would verify the catch branch in maybeArmChannelRetry (meshcoreManager.ts:3108-3111). It's a one-liner to add to the existing test harness:

it('(h) DB error reading setting → retry not armed (fail-safe)', async () => {
  const { manager, bridgeCalls } = makeManager({ retryEnabled: true });
  vi.spyOn(databaseService.settings, 'getSettingAsBoolean').mockRejectedValueOnce(new Error('DB down'));
  await manager.sendMessage('hi', undefined, 0, undefined, true);
  expect((manager as any).pendingChannelRetries.size).toBe(0);
  await vi.advanceTimersByTimeAsync(40_000);
  expect(sends(bridgeCalls)).toHaveLength(1);
});

7. meshActionDeps.test.ts assertions on autoRetryOnMiss=true trailing arg are correct

Lines 34, 44, 74 — updated expectations now assert the trailing true on sendMessage calls from automation actions. This is exactly right.

---

Settings / UI

8. meshcoreChannelRetryEnabled is not in PER_SOURCE_SETTINGS_KEYS

settings.ts:163-168 — The setting is global-by-design (one toggle applies to all MeshCore sources), which is correct behavior. It's added to VALID_SETTINGS_KEYS only, which is the right place. Just confirming this is intentional — the PR description says "new global setting", so this is consistent.

9. settings-meshcore-messaging placed in GLOBAL_SECTIONS set

SettingsTab.tsx:97 — The section is correctly gated as global-only. However, a user with only per-source permissions who navigates to global settings will see a "MeshCore Messaging" section even if they have no MeshCore sources at all. This is not a regression (same UX pattern as Link Previews), but worth a follow-up to optionally hide the section when no MeshCore sources are configured.

10. Inline checkbox in <h3> is unusual

SettingsTab.tsx:1398-1408 — The toggle control is embedded in the <h3> heading element, while other similar toggles (e.g. Link Previews) use a separate <label> + checkbox within a setting-item div. This is not wrong but is visually inconsistent with the rest of the settings page. Minor cosmetic issue.

---

Documentation

The new doc sections in docs/features/meshcore.md and docs/features/automation-engine.md are accurate, appropriately concise, and match the actual behavior. The "opt-in, channel-only, one-shot, distinct from DM retry" characterization is correct.

---

Summary

Category	Finding	Severity
Correctness	CHANNEL_RETRY_WINDOW_MS and HEARD_WINDOW_MS are separate constants that must stay equal	Low (doc/test gap, not a bug today)
Correctness	retriesLeft field is set but never actually used as a decrement guard	Low (dead code, not a bug)
Test coverage	Missing test for DB error → no retry	Low
Observability	heardCount > 0 path logs at debug only; setting read failure is silent to operator	Low
UI	Inline checkbox in <h3> is inconsistent with the rest of the settings page	Cosmetic

No blocking issues. The implementation is solid and well-tested. The two most actionable items before merge would be (1) adding the DB-error-abort test and (2) either unifying the two 30 s constants or adding an explicit assertion/comment about their required equality.