refactor(heartbeat): extract protocol-agnostic HeartbeatScheduler; MeshCore adopts (#3962)

[Yeraze]

Summary

Task 2.2a of the remediation plan (#3962, Phase 2) — with a documented premise correction.

The plan assumed heartbeat/status-probing was duplicated across both managers. Investigation showed otherwise: Meshtastic's "heartbeat" is a fire-and-forget keepalive push living in the transport layer (tcpTransport.ts), MeshCore's is a manager-owned request/response probe loop with failure counting feeding its reconnect machinery, and MQTT has none. There is no shared subsystem to unify — so this PR does the honest minimal version instead of a forced abstraction:

• New src/server/services/heartbeatScheduler.ts — a protocol-agnostic leaf utility owning only the genuinely reusable mechanism: interval management, idempotent start/stop, in-flight guard, and connected-gates pre- and post-await on both the resolve and throw paths (drops late results racing a teardown). Takes a narrow capability interface (probe/isConnected/onSuccess/onFailure) — deliberately not ISourceManager.
• MeshCore adopts it: startHeartbeat/stopHeartbeat become thin delegates; the probe becomes a clean heartbeatProbe() wire op. The failure counter, heartbeat_ok/heartbeat_failed events, threshold logic, and beginReconnect() stay in the manager — the deliberate scope firewall against the reconnect state machine (Phase 4.2's job). Socket-drop detection ([BUG] MeshCore: Cannot connect to virtual node #3705) untouched.
• Meshtastic and MQTT are untouched (verified by grep).
• All 20 gates/timing constants from the current implementation are mapped 1:1 in the PR's behavior-preservation table (in the implementation notes); a new integration test proves 3 consecutive probe failures still drive heartbeat_failed ×3 → connectionState='reconnecting'.

A third source type gets liveness probing for free by supplying four callbacks.

Verification

• 9 fake-timer unit tests on the scheduler (every gate) + new failure→reconnect integration test; existing meshcore heartbeat tests green.
• Independent orchestrator run: full suite 8277 passed, 0 failed, 0 suite failures (success: true); typecheck clean; test-typecheck at 282 baseline; npm run lint:ci green.

Also checks off 2.1 in the epic status doc.

Refs #3962

🤖 Generated with Claude Code

https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Post review feedback

---

Overall this is a well-structured, clean extraction. The HeartbeatScheduler is genuinely protocol-agnostic, the gate logic is correct, and the tests are comprehensive. A few observations below, ranging from actual bugs to nitpicks.

---

Potential Bug: heartbeatProbe never returns false

src/server/meshcoreManager.ts:5670–5677

private async heartbeatProbe(timeoutMs: number): Promise<boolean> {
  if (!this.nativeBackend) {
    throw new Error('no native backend');
  }
  const response = await this.nativeBackend.sendCommand('get_device_time', {}, timeoutMs);
  if (response.success) return true;
  throw new Error(response.error ?? 'probe failed');  // ← always throws, never returns false
}

The HeartbeatScheduler interface docs say the probe can "Resolve false → probe reported failure". But this implementation never resolves false — it always throws on failure. That's valid per the scheduler's contract (throw → onFailure), but the test at meshcoreNativeBackend.test.ts:1326 uses a getDeviceTime that throws, and test 6a in the scheduler suite exercises false. This mismatch between what's documented on the interface and what the actual caller does isn't a bug today, but if a future implementer reads the probe docs and writes a false-returning probe while assuming the manager handles it, they'll be confused. Consider either: (a) making the probe clearly throw-only by changing the return type to Promise<void>, or (b) adding a test that covers the false path at the manager level.

---

Minor: startHeartbeat sets shouldReconnect = true as a side effect

src/server/meshcoreManager.ts:5645

private startHeartbeat(): void {
  const intervalSecs = this.config?.heartbeatIntervalSeconds ?? 0;
  if (intervalSecs <= 0) return;
  if (this.heartbeatScheduler?.running) return;
  this.shouldReconnect = true;  // ← side effect buried in heartbeat start
  ...
}

This is correct behavior (enabling heartbeat also enables auto-reconnect), but the side effect is non-obvious: a caller that only reads startHeartbeat() won't realize it's also toggling reconnect state. A comment linking this to the "shouldReconnect + heartbeat co-enable" behavior would help. (This code predates the PR, but the refactor is a good moment to document it.)

---

Timing concern: stop() clears probeInFlight unconditionally

src/server/services/heartbeatScheduler.ts:76

stop(): void {
  if (this.timer !== null) {
    clearInterval(this.timer);
    this.timer = null;
  }
  this.probeInFlight = false;  // ← cleared even if probe is still awaiting
}

If stop() is called while a probe await is in progress, the in-flight guard is cleared. The next start() → runProbe() could then fire a second concurrent probe before the original resolves. In practice this is guarded by beginReconnect() calling stopHeartbeat() and then never calling startHeartbeat() again until connect() (where stopHeartbeat() is also called first), so the double-fire scenario doesn't arise in current usage. But if someone later calls start() after stop() while a slow probe is mid-flight, they'll get overlapping probes. Worth a comment acknowledging this, or setting probeInFlight = false only after ensuring any in-flight probe is cancelled (which isn't straightforward without an AbortController, so a comment documenting the assumption is probably the right call).

---

Test: integration test uses real timers with setTimeout(r, 3500)

src/server/meshcoreNativeBackend.test.ts:1352

await new Promise((r) => setTimeout(r, 3500));

The integration test at line 1325 uses real timers with a 3.5-second sleep to wait for 3 probe cycles (1s each). This makes the test slow and subject to CI timing variance. The earlier unit tests wisely use vi.useFakeTimers() + advanceTimersByTimeAsync. Consider doing the same here for determinism — though the tradeoff is that MeshCoreManager likely uses setInterval internally via HeartbeatScheduler, so fake timers would need to cover that too, which may require additional setup.

---

Nit: onFailure callback called with new Error('probe failed') (generic message)

src/server/services/heartbeatScheduler.ts:100

this.opts.onFailure(new Error('probe failed'));

When the probe returns false (as opposed to throwing), the error message is always the generic string 'probe failed'. This matches what the manager currently does (it always throws), but if the scheduler is reused by a future caller that returns false with protocol-specific context, the caller's onFailure handler gets a generic error and loses diagnostics. Consider accepting an optional message from the probe return: e.g. Promise<boolean | string> where a string means "failed with this reason". Low priority since the current caller always throws.

---

Test coverage: no test for stop() during mid-flight probe then start() again

The stop-mid-flight → restart scenario described in the timing concern above has no test. Given that probeInFlight is cleared by stop(), a test would help document the intended behavior and catch a future regression if the restart semantics change.

---

Strengths

• Gate logic is correct: pre-gate and post-gate (on both resolve and throw paths) using isConnected() are all independently tested (tests 4, 5, 6b).
• Idempotent start/stop: tested (test 2) and the guard in startHeartbeat() checks scheduler.running before creating a new instance.
• onFailure callback swallow test (test 8): an excellent edge case — a misbehaving callback shouldn't crash the interval loop.
• Clear separation of concerns: reconnect state machine, failure counter, events all stay in MeshCoreManager; the scheduler owns only the timing mechanism.
• heartbeatProbe as a thin wire op with the label + timeout forwarded cleanly.
• PR description: the behavior-preservation table and premise correction are well-documented.

---

Summary

Approachable refactor with one real concern (potential overlapping probes after stop()+start() with in-flight probe, though not currently reachable) and one API clarity issue (probe contract allows false but implementation never uses it). The integration test slowness (3.5s wall-clock) is worth addressing eventually. Everything else is minor polish.