From 86fe35ced503ee84d5c76a833a7c42555ecc5859 Mon Sep 17 00:00:00 2001 From: Joe WB3IHY Date: Tue, 4 Aug 2026 22:00:25 -0400 Subject: [PATCH 1/5] fix(meshtastic): harden TCP reconnect against stuck queue, slow detection, and hung attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three compounding issues surfaced during live TCP reconnect testing against a flaky node: - @jsr/meshtastic__core's Queue.processQueue() retried a failed write on the same first-in-line packet forever at a hardcoded 200ms cadence, holding its internal lock and starving every other queued packet (including 60s heartbeats) behind it. Queue.remove() then deadlocked trying to clear that same packet, since it refused to run while the lock it was waiting on was held. One transient write failure (e.g. socket briefly gone mid-reconnect) permanently wedged that device's outbound queue. Patched via pnpm patchedDependencies: a failed write now marks the packet attempted so the loop can move on, and remove() no longer gates on the lock (queue mutation is synchronous/single-threaded, so there was never a real race to guard). - TCP disconnects were only ever noticed by the passive stale/dead watchdog, which for TCP shares serial's conservative 120s/180s thresholds — up to 3 minutes of a dead connection before any reconnect was attempted. Main already reports the socket's own close/error event within milliseconds; meshtasticTransportLossDetection.ts now also subscribes to meshtastic:tcp-disconnected directly and triggers reconnect immediately, mirroring the existing serial unplug fast path. - Reconnect attempts had no ceiling for TCP/HTTP/serial (only BLE had raceWithDeadline + NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS). Faster disconnect detection makes back-to-back drops far more likely to land while a prior attempt is still opening; if that attempt then hangs (observed: a device dying mid-configure), the reconnect state machine had nothing to unstick it and stayed wedged indefinitely. raceWithDeadline is now applied to every transport, not just BLE. --- patches/@jsr__meshtastic__core@2.6.6.patch | 71 +++++++++++++++++++ pnpm-lock.yaml | 10 +-- .../meshtasticTransportLossDetection.test.ts | 53 ++++++++++++++ .../meshtasticTransportLossDetection.ts | 12 ++++ ...htasticRuntime.reconnect-hardening.test.ts | 8 ++- src/renderer/runtime/useMeshtasticRuntime.ts | 20 +++--- vitest.config.mts | 1 + 7 files changed, 159 insertions(+), 16 deletions(-) diff --git a/patches/@jsr__meshtastic__core@2.6.6.patch b/patches/@jsr__meshtastic__core@2.6.6.patch index ee3e66a72..9231c2c86 100644 --- a/patches/@jsr__meshtastic__core@2.6.6.patch +++ b/patches/@jsr__meshtastic__core@2.6.6.patch @@ -127,3 +127,74 @@ index ddca80fc9134fa56ac273b17387e62af087c95c6..365b64b0e60cce8f23cb9d6a6d463b6d await this.transport.disconnect(); } +diff --git a/src/utils/queue.js b/src/utils/queue.js +index d6082a6401e15eeb3e8873820b64341c961ac35f..73457e9e6fbde50800af427bb82b53b59049a7df 100755 +--- a/src/utils/queue.js ++++ b/src/utils/queue.js +@@ -55,10 +55,8 @@ export class Queue { + this.queue.push(queueItem); + } + remove(id) { +- if (this.lock) { +- setTimeout(()=>this.remove(id), 100); +- return; +- } ++ // mesh-client patch: see src/utils/queue.ts for rationale — this used to deadlock ++ // whenever processQueue never released `lock`. + this.queue = this.queue.filter((item)=>item.id !== id); + } + processAck(id) { +@@ -88,10 +86,12 @@ export class Queue { + await new Promise((resolve)=>setTimeout(resolve, 200)); + try { + await writer.write(item.data); +- item.sent = true; + } catch (error) { + console.error(`Error sending packet ${item.id}`, error); + } ++ // mesh-client patch: see src/utils/queue.ts for rationale — mark attempted either ++ // way so one write failure can't wedge every other queued packet behind it. ++ item.sent = true; + } + } + } finally{ +diff --git a/src/utils/queue.ts b/src/utils/queue.ts +index 9c0d192d0a22c1a158e70f4dc3b74460ea7bd22b..7d63cb22ad5c6a8cde8823b9925e93b63f8cdeb9 100755 +--- a/src/utils/queue.ts ++++ b/src/utils/queue.ts +@@ -70,10 +70,11 @@ export class Queue { + } + + public remove(id: number): void { +- if (this.lock) { +- setTimeout(() => this.remove(id), 100); +- return; +- } ++ // mesh-client patch: `queue` mutation is synchronous and single-threaded, so there is ++ // no real race to guard against here. Gating on `lock` deadlocked whenever ++ // `processQueue` never released it (see the processQueue patch below): the item's own ++ // 60s ack/timeout in push() could never actually remove it, so processQueue kept ++ // re-picking the same still-queued item forever. + this.queue = this.queue.filter((item) => item.id !== id); + } + +@@ -115,10 +116,18 @@ export class Queue { + await new Promise((resolve) => setTimeout(resolve, 200)); + try { + await writer.write(item.data); +- item.sent = true; + } catch (error) { + console.error(`Error sending packet ${item.id}`, error); + } ++ // mesh-client patch: mark the item attempted whether or not the write succeeded. ++ // The original code only did this on success, so a single transport-level write ++ // failure (e.g. socket briefly gone during a reconnect) left the item permanently ++ // unsent — this loop kept re-picking the same first-in-line item forever at the ++ // 200ms cadence above, holding `lock` and starving every other queued packet ++ // (including heartbeats) behind it. The item's own promise still settles normally ++ // via the ack/60s-timeout machinery in push(); this only stops it from wedging ++ // every *other* packet. ++ item.sent = true; + } + } + } finally { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b5f4a21e0..8077addea 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -25,7 +25,7 @@ overrides: ws: ^8.21.0 patchedDependencies: - '@jsr/meshtastic__core@2.6.6': de16755c559d2387594c6bdcd76c7e643f15af1b8398fc1fd3e067fa93f7be6a + '@jsr/meshtastic__core@2.6.6': 93604a080fa754cbde3dc78ef46ee50c2996c34d6d85720acacecfd6ebd7c652 '@jsr/meshtastic__transport-web-serial@0.2.5': 27a2418bae8605e0e5ab6f1fcfd391abd9dc3110433da5754e934f38475dab74 '@liamcottle/meshcore.js@1.13.0': 1f95835871af6026efa9def5d5bed54749cdb34ff0ff013a9d52005db9db3220 debug@4.4.3: cf37fa96f5df733456b16c82c9e1c9054a92f6216692aa33d5e2e1e208888e37 @@ -129,7 +129,7 @@ importers: version: 1.13.0(patch_hash=1f95835871af6026efa9def5d5bed54749cdb34ff0ff013a9d52005db9db3220)(supports-color@8.1.1) '@meshtastic/core': specifier: npm:@jsr/meshtastic__core@^2.6.6 - version: '@jsr/meshtastic__core@2.6.6(patch_hash=de16755c559d2387594c6bdcd76c7e643f15af1b8398fc1fd3e067fa93f7be6a)(buffer@6.0.3)' + version: '@jsr/meshtastic__core@2.6.6(patch_hash=93604a080fa754cbde3dc78ef46ee50c2996c34d6d85720acacecfd6ebd7c652)(buffer@6.0.3)' '@meshtastic/transport-http': specifier: npm:@jsr/meshtastic__transport-http@^0.2.1 version: '@jsr/meshtastic__transport-http@0.2.1(buffer@6.0.3)' @@ -5243,7 +5243,7 @@ snapshots: '@jridgewell/resolve-uri': 3.1.2 '@jridgewell/sourcemap-codec': 1.5.5 - '@jsr/meshtastic__core@2.6.6(patch_hash=de16755c559d2387594c6bdcd76c7e643f15af1b8398fc1fd3e067fa93f7be6a)(buffer@6.0.3)': + '@jsr/meshtastic__core@2.6.6(patch_hash=93604a080fa754cbde3dc78ef46ee50c2996c34d6d85720acacecfd6ebd7c652)(buffer@6.0.3)': dependencies: '@bufbuild/protobuf': 2.13.0 '@jsr/meshtastic__protobufs': 2.7.26 @@ -5259,13 +5259,13 @@ snapshots: '@jsr/meshtastic__transport-http@0.2.1(buffer@6.0.3)': dependencies: - '@jsr/meshtastic__core': 2.6.6(patch_hash=de16755c559d2387594c6bdcd76c7e643f15af1b8398fc1fd3e067fa93f7be6a)(buffer@6.0.3) + '@jsr/meshtastic__core': 2.6.6(patch_hash=93604a080fa754cbde3dc78ef46ee50c2996c34d6d85720acacecfd6ebd7c652)(buffer@6.0.3) transitivePeerDependencies: - buffer '@jsr/meshtastic__transport-web-serial@0.2.5(patch_hash=27a2418bae8605e0e5ab6f1fcfd391abd9dc3110433da5754e934f38475dab74)(buffer@6.0.3)': dependencies: - '@meshtastic/core': '@jsr/meshtastic__core@2.6.6(patch_hash=de16755c559d2387594c6bdcd76c7e643f15af1b8398fc1fd3e067fa93f7be6a)(buffer@6.0.3)' + '@meshtastic/core': '@jsr/meshtastic__core@2.6.6(patch_hash=93604a080fa754cbde3dc78ef46ee50c2996c34d6d85720acacecfd6ebd7c652)(buffer@6.0.3)' transitivePeerDependencies: - buffer diff --git a/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.test.ts b/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.test.ts index ce7c11b65..d89a1bcd7 100644 --- a/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.test.ts +++ b/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.test.ts @@ -63,6 +63,59 @@ describe('meshtasticTransportLossDetection', () => { expect(onLost).toHaveBeenCalledTimes(1); }); + it('notifies immediately on main-process TCP socket disconnect', () => { + // Regression: writes that fail after the socket is gone report "no active socket", + // which does not match TRANSPORT_LOST_MESSAGE, so this IPC event is TCP's only fast + // path — without it, TCP relied solely on the passive watchdog (up to 3 minutes). + let capturedCb: (() => void) | undefined; + const spy = vi + .spyOn(window.electronAPI.meshtastic.tcp, 'onDisconnected') + .mockImplementation((cb) => { + capturedCb = cb; + return () => {}; + }); + + const onLost = vi.fn(); + const inner = new WritableStream({ write: vi.fn() }); + const device = { + transport: { toDevice: inner }, + } as unknown as MeshDevice; + + try { + attachMeshtasticTransportLossWatch(device, 'tcp', onLost); + expect(spy).toHaveBeenCalledTimes(1); + + capturedCb?.(); + + expect(onLost).toHaveBeenCalledTimes(1); + } finally { + spy.mockRestore(); + } + }); + + it('unsubscribes the TCP disconnect listener on cleanup', () => { + const unsub = vi.fn(); + const spy = vi + .spyOn(window.electronAPI.meshtastic.tcp, 'onDisconnected') + .mockReturnValue(unsub); + + const inner = new WritableStream({ write: vi.fn() }); + const device = { + transport: { toDevice: inner }, + } as unknown as MeshDevice; + + try { + const detach = attachMeshtasticTransportLossWatch(device, 'tcp', vi.fn()); + expect(unsub).not.toHaveBeenCalled(); + + detach(); + + expect(unsub).toHaveBeenCalledTimes(1); + } finally { + spy.mockRestore(); + } + }); + it('serializes concurrent getWriter calls without WritableStream locked errors', async () => { let innerWriteCount = 0; const inner = new WritableStream({ diff --git a/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.ts b/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.ts index d8e62146f..7b2438986 100644 --- a/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.ts +++ b/src/renderer/lib/meshtastic/meshtasticTransportLossDetection.ts @@ -195,6 +195,18 @@ export function attachMeshtasticTransportLossWatch( } } + if (type === 'tcp') { + // Main process reports the socket's own 'close'/'error' event within milliseconds of the + // real network failure (clean FIN or RST alike). Without this, TCP relied solely on the + // passive stale/dead watchdog noticing silence — up to 3 minutes after a connection that + // was already gone — because writes that fail with "no active socket" don't match the + // TRANSPORT_LOST_MESSAGE regex below, so this is the only fast path for TCP. + const unsubTcpDisconnected = window.electronAPI.meshtastic.tcp.onDisconnected(() => { + notify('tcp-socket-closed'); + }); + cleanups.push(unsubTcpDisconnected); + } + const transport = device.transport as { toDevice?: WritableStream } | undefined; if (transport?.toDevice) { const transportObj = device.transport as object; diff --git a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts index 1a75bc1cb..2e7016f17 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts @@ -124,13 +124,17 @@ describe('useMeshtasticRuntime reconnect hardening (regression)', () => { expect(reconnectBody).toContain('skip overlapping open'); }); - it('bounds BLE reconnect open+configure with NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS', () => { + it('bounds every reconnect open+configure with NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS', () => { + // Applies to all transports, not just BLE (see comment at the call site): TCP/HTTP/serial + // used to await the open+configure attempt with no ceiling at all, so a hang anywhere in + // that sequence (e.g. a disconnect landing mid-configure) wedged reconnection forever. expect(SOURCE).toContain('NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS'); expect(SOURCE).toContain('raceWithDeadline'); const reconnectBody = extractUseCallbackBody(SOURCE, 'attemptReconnect'); expect(reconnectBody).toContain('raceWithDeadline'); - expect(reconnectBody).toContain('BLE reconnect attempt timed out after'); + expect(reconnectBody).toContain('Reconnect attempt timed out after'); expect(reconnectBody).toContain('attemptActive'); + expect(reconnectBody).not.toContain('if (isBleReconnect) {'); }); it('disconnects late-opened transport when reconnect attempt is inactive or superseded', () => { diff --git a/src/renderer/runtime/useMeshtasticRuntime.ts b/src/renderer/runtime/useMeshtasticRuntime.ts index 70a6506d3..8f11c59c3 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.ts @@ -2255,15 +2255,17 @@ export function useMeshtasticRuntime() { // Late loser after budget timeout — avoid unhandledRejection; cleanup is in catch / late path. void reconnectWork.catch(() => {}); try { - if (isBleReconnect) { - await raceWithDeadline( - reconnectWork, - NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS, - `BLE reconnect attempt timed out after ${NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS}ms`, - ); - } else { - await reconnectWork; - } + // Applied to every transport, not just BLE (constant name is historical): TCP/HTTP/serial + // reconnects used to `await reconnectWork` with no ceiling at all. A disconnect that lands + // while an open+configure is still in flight (now common — TCP disconnect detection is + // near-instant, see meshtasticTransportLossDetection.ts) defers to that attempt settling; + // without a deadline here, a hang anywhere in openMeshtasticTransport/configure wedges the + // whole reconnect state machine forever instead of just failing this attempt and retrying. + await raceWithDeadline( + reconnectWork, + NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS, + `Reconnect attempt timed out after ${NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS}ms`, + ); } catch (err) { attemptActive = false; const failedDriverIdentity = diff --git a/vitest.config.mts b/vitest.config.mts index b01b63027..5edefbf18 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -100,6 +100,7 @@ const RENDERER_LOGIC_EXCLUDE = [ 'src/renderer/lib/meshtasticRemoteAdminKeyStorage.test.ts', 'src/renderer/lib/meshcore/meshcoreLiveContactPersist.test.ts', 'src/renderer/lib/meshtastic/meshtasticTransportSideEffects.test.ts', + 'src/renderer/lib/meshtastic/meshtasticTransportLossDetection.test.ts', 'src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.post-reboot.test.ts', 'src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.telemetry-nodeinfo.test.ts', 'src/renderer/lib/meshtastic/meshtasticNodeSideEffects.test.ts', From b27480fb0e0a7dbaa69539213109ccb9f4b9db6d Mon Sep 17 00:00:00 2001 From: Joe WB3IHY Date: Tue, 4 Aug 2026 22:09:16 -0400 Subject: [PATCH 2/5] fix(meshcore): add TCP disconnect detection and reconnect deadline parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Auditing MeshCore's TCP path for the same class of issues just fixed for Meshtastic (requested during review) turned up two of the three, and one gap worse than Meshtastic ever had: - No automatic recovery path for TCP at all. The only transport-loss-watch in useMeshcoreRuntime.ts (attachMeshcoreSerialTransportLossWatch) and the only watchdog (startMeshcoreSerialWatchdog) are both gated on rfType === 'serial'. meshcore:tcp-disconnected already fires from main on socket close/error, but the renderer only fed it into the SDK's own internal bookkeeping and a cosmetic device_status store update — nothing triggered reconnect. A dropped MeshCore TCP connection would sit disconnected in the UI forever. Added a disconnect-triggered reconnect mirroring the existing serial pattern. - Same missing reconnect-attempt deadline as Meshtastic: only BLE had raceWithDeadline + NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS; TCP/serial awaited the open+attach attempt with no ceiling. Now applied to every transport. @liamcottle/meshcore.js has no analog to the Meshtastic SDK's Queue class (no internal write queue/retry at all — every send is a single direct write), so the queue-deadlock fix has no MeshCore equivalent to make. Not field-tested against real MeshCore hardware — no physical node available. Code mirrors the now-verified Meshtastic pattern closely and is covered by source-contract tests, but behavior should be confirmed against a live MeshCore TCP node before this is fully trusted. --- .../useMeshcoreRuntime.reconnect.test.ts | 18 ++++++++-- src/renderer/runtime/useMeshcoreRuntime.ts | 33 ++++++++++++++----- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts b/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts index 91f26fe1d..dde6ac589 100644 --- a/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts +++ b/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts @@ -108,14 +108,19 @@ describe('useMeshcoreRuntime auto-reconnect (regression)', () => { ); }); - it('bounds BLE reconnect open+attach with NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS', () => { + it('bounds every reconnect open+attach with NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS', () => { + // Applies to all transports, not just BLE (see comment at the call site): TCP/serial used + // to await the open+attach attempt with no ceiling at all, so a hang anywhere in that + // sequence (e.g. a disconnect landing mid-attach) wedged reconnection forever — and unlike + // serial, MeshCore TCP has no fallback watchdog either (see meshcoreSerialWatchdog). expect(RUNTIME_SOURCE).toContain('NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS'); expect(RUNTIME_SOURCE).toContain('raceWithDeadline'); const reconnectBody = extractUseCallbackBody(RUNTIME_SOURCE, 'attemptMeshcoreReconnect'); expect(reconnectBody).toContain('raceWithDeadline'); - expect(reconnectBody).toContain('BLE reconnect attempt timed out after'); + expect(reconnectBody).toContain('Reconnect attempt timed out after'); expect(reconnectBody).toContain('attemptActive'); expect(reconnectBody).toContain('meshcoreReconnectConnectInFlightRef.current = true'); + expect(reconnectBody).not.toContain('if (isBleReconnect) {\n await raceWithDeadline'); }); it('on BLE reconnect timeout invalidates setup generation and cleans late transports', () => { @@ -242,6 +247,15 @@ describe('useMeshcoreRuntime auto-reconnect (regression)', () => { expect(RUNTIME_SOURCE).toContain('startSerialRediscovery'); expect(RUNTIME_SOURCE).toContain('captureSerialIdentityForRediscovery'); }); + + it('notifies immediately on main-process TCP socket disconnect (regression)', () => { + // Unlike serial, MeshCore's TCP transport has no fallback watchdog at all (see + // startMeshcoreSerialWatchdog, gated on rfType === 'serial'), so meshcore.tcp.onDisconnected + // is the only automatic recovery path for a dropped TCP connection. + expect(RUNTIME_SOURCE).toMatch( + /window\.electronAPI\.meshcore\.tcp\.onDisconnected\(\(\) => \{[\s\S]*?rfType !== 'tcp'[\s\S]{0,200}handleMeshcoreConnectionLostRef\.current\(\)/, + ); + }); }); describe('useMeshcoreRuntime manual disconnect must not auto-reconnect', () => { diff --git a/src/renderer/runtime/useMeshcoreRuntime.ts b/src/renderer/runtime/useMeshcoreRuntime.ts index 142d0b5cb..92702248e 100644 --- a/src/renderer/runtime/useMeshcoreRuntime.ts +++ b/src/renderer/runtime/useMeshcoreRuntime.ts @@ -2875,15 +2875,16 @@ export function useMeshcoreRuntime() { const reconnectWork = runReconnectAttempt(); void reconnectWork.catch(() => {}); try { - if (isBleReconnect) { - await raceWithDeadline( - reconnectWork, - NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS, - `BLE reconnect attempt timed out after ${NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS}ms`, - ); - } else { - await reconnectWork; - } + // Applied to every transport, not just BLE (constant name is historical): TCP/serial + // reconnects used to `await reconnectWork` with no ceiling at all. A disconnect that + // lands while an open+attach is still in flight defers to that attempt settling; without + // a deadline here, a hang anywhere in openMeshCoreTransport/attachRfSession wedges the + // whole reconnect state machine forever instead of just failing this attempt and retrying. + await raceWithDeadline( + reconnectWork, + NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS, + `Reconnect attempt timed out after ${NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS}ms`, + ); } catch (err) { attemptActive = false; if (isBleReconnect) { @@ -6848,6 +6849,20 @@ export function useMeshcoreRuntime() { return () => registerMeshcoreSerialDisconnectTarget(null); }, []); + // Main reports the raw TCP socket's own 'close'/'error' event within milliseconds of the + // real network failure (clean FIN or RST alike). Without this, TCP relied solely on the + // passive stale/dead watchdog — which, unlike serial, doesn't exist for MeshCore's TCP + // transport at all (see startMeshcoreSerialWatchdog), so a dropped TCP connection had no + // automatic recovery path whatsoever. Mirrors the equivalent Meshtastic fix in + // meshtasticTransportLossDetection.ts. + useEffect(() => { + const unsub = window.electronAPI.meshcore.tcp.onDisconnected(() => { + if (meshcoreConnectionParamsRef.current?.rfType !== 'tcp') return; + handleMeshcoreConnectionLostRef.current(); + }); + return () => unsub(); + }, []); + useEffect(() => { registerMeshcoreSession({ prepareRfConnect, From 1ab6251ccac7c6eeb8f379beefec92a15332de00 Mon Sep 17 00:00:00 2001 From: Joe WB3IHY Date: Tue, 4 Aug 2026 22:33:45 -0400 Subject: [PATCH 3/5] fix: address CodeRabbit findings on reconnect-deadline cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real gaps, both consequences of extending raceWithDeadline to every transport instead of just BLE: - useMeshtasticRuntime.ts: when a reconnect attempt times out, wireSubscriptions() has already run (it's called synchronously right after open, well before the deadline can fire) — so the loss-watch listener and wrapped toDevice stream stayed live against the abandoned device. lateTransport.cleanup() only tears down the driver/transport, not those. Now calls cleanupSubscriptions() too. - useMeshcoreRuntime.ts: meshcoreSetupGenerationRef invalidation on timeout (stops background initConn RPCs — getSelfInfo/getContacts/getChannels/etc. — from applying stale state after an attempt is declared failed) was still gated on isBleReconnect, left over from when only BLE could ever hit this catch path via the deadline. A timed-out TCP/serial attempt now invalidates the setup generation too. Both flagged by CodeRabbit on #792; the MeshCore one is exactly the class of issue raised in review (BLE-specific race handling not exercised against TCP until raceWithDeadline applied to it) — meshcoreSetupGenerationRef is used generically elsewhere in the file, so gating its bump here on isBleReconnect was already narrow even before this PR, just never reachable for non-BLE. --- .../runtime/useMeshcoreRuntime.reconnect.test.ts | 15 ++++++++++++--- src/renderer/runtime/useMeshcoreRuntime.ts | 10 ++++++---- ...eMeshtasticRuntime.reconnect-hardening.test.ts | 12 ++++++++++++ src/renderer/runtime/useMeshtasticRuntime.ts | 5 +++++ 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts b/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts index dde6ac589..9b2de01a5 100644 --- a/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts +++ b/src/renderer/runtime/useMeshcoreRuntime.reconnect.test.ts @@ -123,7 +123,13 @@ describe('useMeshcoreRuntime auto-reconnect (regression)', () => { expect(reconnectBody).not.toContain('if (isBleReconnect) {\n await raceWithDeadline'); }); - it('on BLE reconnect timeout invalidates setup generation and cleans late transports', () => { + it('on reconnect timeout invalidates setup generation and cleans late transports (any transport, CodeRabbit #792)', () => { + // meshcoreSetupGenerationRef guards background initConn RPCs (getSelfInfo/getContacts/ + // getChannels/etc.) generically — not BLE-specific (see its other call sites). Gating the + // bump on isBleReconnect here was only ever correct while raceWithDeadline itself was + // BLE-only; now that every transport's reconnect races the same deadline, a timed-out + // TCP/serial attempt must invalidate the setup generation too, or its background RPCs keep + // running and can apply stale state after the attempt was already declared failed. expect(RUNTIME_SOURCE).toContain('createBleReconnectTransportCleanup'); const reconnectBody = extractUseCallbackBody(RUNTIME_SOURCE, 'attemptMeshcoreReconnect'); expect(reconnectBody).toMatch( @@ -132,9 +138,12 @@ describe('useMeshcoreRuntime auto-reconnect (regression)', () => { expect(reconnectBody).toMatch( /lateTransport\.cleanup\(opened\.driverIdentityId\);\s*throw new Error\('MeshCore reconnect superseded during attach'\)/, ); - expect(reconnectBody).toMatch( - /catch \(err\) \{[\s\S]*?isBleReconnect[\s\S]*?meshcoreSetupGenerationRef\.current \+= 1/, + const catchBody = reconnectBody.slice( + reconnectBody.indexOf('} catch (err) {'), + reconnectBody.indexOf('await lateTransport.cleanup(opened?.driverIdentityId)'), ); + expect(catchBody).toContain('meshcoreSetupGenerationRef.current += 1'); + expect(catchBody).not.toContain('if (isBleReconnect)'); }); it('cleans up transport when RF link is lost after reconnect attach', () => { diff --git a/src/renderer/runtime/useMeshcoreRuntime.ts b/src/renderer/runtime/useMeshcoreRuntime.ts index 92702248e..fe2c5a0ae 100644 --- a/src/renderer/runtime/useMeshcoreRuntime.ts +++ b/src/renderer/runtime/useMeshcoreRuntime.ts @@ -2887,10 +2887,12 @@ export function useMeshcoreRuntime() { ); } catch (err) { attemptActive = false; - if (isBleReconnect) { - // Stop background initConn RPCs if open resolved into attach after the budget fired. - meshcoreSetupGenerationRef.current += 1; - } + // Stop background initConn RPCs (getSelfInfo/getContacts/getChannels/etc.) if open + // resolved into attach after the budget fired. Not BLE-specific: raceWithDeadline now + // guards every transport's reconnect attempt, so a TCP/serial attempt can hit this same + // path — bumping only for BLE here left non-BLE stale setup RPCs free to keep running + // and apply state after the attempt was already declared failed. + meshcoreSetupGenerationRef.current += 1; if (isMeshcoreSetupAbortError(err)) { console.debug('[useMeshcoreRuntime] reconnect aborted (setup superseded)'); meshcoreIsReconnectingRef.current = false; diff --git a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts index 2e7016f17..7faab5231 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts @@ -137,6 +137,18 @@ describe('useMeshtasticRuntime reconnect hardening (regression)', () => { expect(reconnectBody).not.toContain('if (isBleReconnect) {'); }); + it('detaches wire subscriptions when a reconnect attempt times out (CodeRabbit #792)', () => { + // wireSubscriptions() runs synchronously right after open, well before the deadline can + // fire, so a timed-out attempt leaves the loss-watch listener and wrapped toDevice stream + // live against the now-abandoned device unless the deadline's own catch block detaches them + // too — lateTransport.cleanup() alone only tears down the driver/transport, not those. + const reconnectBody = extractUseCallbackBody(SOURCE, 'attemptReconnect'); + const cleanupIdx = reconnectBody.indexOf('await lateTransport.cleanup(failedDriverIdentity);'); + expect(cleanupIdx).toBeGreaterThan(-1); + const afterCleanup = reconnectBody.slice(cleanupIdx, cleanupIdx + 500); + expect(afterCleanup).toContain('cleanupSubscriptions();'); + }); + it('disconnects late-opened transport when reconnect attempt is inactive or superseded', () => { expect(SOURCE).toContain('createBleReconnectTransportCleanup'); const reconnectBody = extractUseCallbackBody(SOURCE, 'attemptReconnect'); diff --git a/src/renderer/runtime/useMeshtasticRuntime.ts b/src/renderer/runtime/useMeshtasticRuntime.ts index 8f11c59c3..f798d73ce 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.ts @@ -2276,6 +2276,11 @@ export function useMeshtasticRuntime() { meshtasticDriverConnectedRef.current = false; meshtasticPendingDriverIdentityRef.current = null; await lateTransport.cleanup(failedDriverIdentity); + // wireSubscriptions() already ran for this attempt's device by the time raceWithDeadline + // can time out (it's called synchronously right after open, before any long await); if we + // don't detach here, the loss-watch listener and wrapped toDevice stream stay live against + // the now-abandoned device until something else happens to tear them down. + cleanupSubscriptions(); console.warn( `[useMeshtasticRuntime] Reconnect attempt ${reconnectAttemptRef.current} failed:` + ' ' + From 25ced383764fd34eea9eadd811304cccbf7856bf Mon Sep 17 00:00:00 2001 From: Joey Stanford Date: Tue, 4 Aug 2026 21:02:14 -0600 Subject: [PATCH 4/5] fix: suppress stale TCP disconnect IPC on superseded sockets connect/disconnect clear the active socket ref before destroy(); only emit *-tcp-disconnected when the closing socket is still current so renderer reconnect (#792) does not treat teardown/replace closes as live link drops. --- src/main/index.ipc-security.test.ts | 35 +++++++++++++++++++++++++++++ src/main/index.ts | 18 +++++++++++---- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/main/index.ipc-security.test.ts b/src/main/index.ipc-security.test.ts index a4bfd5489..c55aefc09 100644 --- a/src/main/index.ipc-security.test.ts +++ b/src/main/index.ipc-security.test.ts @@ -119,6 +119,25 @@ describe('meshtastic:tcp-write byte validation (source contract)', () => { const handlerBody = INDEX_SOURCE.slice(handlerIdx, handlerIdx + 1200); expect(handlerBody).toContain('meshtasticTcpSocket.destroy()'); }); + + it('emits meshtastic:tcp-disconnected only for the active socket (PR #792)', () => { + // connect/disconnect null the ref before destroy(); a superseded close must not broadcast + // or the renderer TCP loss-watch will tear down a healthy replacement session. + const handlerIdx = INDEX_SOURCE.indexOf("ipcMain.handle('meshtastic:tcp-connect'"); + expect(handlerIdx).toBeGreaterThan(-1); + const closeIdx = INDEX_SOURCE.indexOf("socket.on('close'", handlerIdx); + expect(closeIdx).toBeGreaterThan(handlerIdx); + const closeBody = INDEX_SOURCE.slice(closeIdx, closeIdx + 600); + expect(closeBody).toContain('if (meshtasticTcpSocket === socket)'); + expect(closeBody).toContain("mainWindow?.webContents.send('meshtastic:tcp-disconnected')"); + // Emit must be inside the active-socket guard (not before it). + const guardIdx = closeBody.indexOf('if (meshtasticTcpSocket === socket)'); + const emitIdx = closeBody.indexOf( + "mainWindow?.webContents.send('meshtastic:tcp-disconnected')", + ); + expect(guardIdx).toBeGreaterThan(-1); + expect(emitIdx).toBeGreaterThan(guardIdx); + }); }); // ─── meshcore:tcp-write byte element validation ────────────────────── @@ -323,6 +342,22 @@ describe('meshcore:tcp-connect hostname validation (source contract)', () => { const handlerBody = INDEX_SOURCE.slice(handlerIdx, handlerIdx + 800); expect(handlerBody).toContain('formatHostForSocket('); }); + + it('emits meshcore:tcp-disconnected only for the active socket (PR #792)', () => { + // Same contract as meshtastic:tcp-connect — superseded closes from connect-replace / + // disconnect must not look like a live link drop to the renderer reconnect path. + const handlerIdx = INDEX_SOURCE.indexOf("ipcMain.handle('meshcore:tcp-connect'"); + expect(handlerIdx).toBeGreaterThan(-1); + const closeIdx = INDEX_SOURCE.indexOf("socket.on('close'", handlerIdx); + expect(closeIdx).toBeGreaterThan(handlerIdx); + const closeBody = INDEX_SOURCE.slice(closeIdx, closeIdx + 600); + expect(closeBody).toContain('if (meshcoreTcpSocket === socket)'); + expect(closeBody).toContain("mainWindow?.webContents.send('meshcore:tcp-disconnected')"); + const guardIdx = closeBody.indexOf('if (meshcoreTcpSocket === socket)'); + const emitIdx = closeBody.indexOf("mainWindow?.webContents.send('meshcore:tcp-disconnected')"); + expect(guardIdx).toBeGreaterThan(-1); + expect(emitIdx).toBeGreaterThan(guardIdx); + }); }); // ─── meshtastic:tcp-connect hostname validation ────────────────────── diff --git a/src/main/index.ts b/src/main/index.ts index edd089ae3..a8e925bb7 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -6123,8 +6123,13 @@ ipcMain.handle('meshcore:tcp-connect', (event, host: string, port: number) => { socket.on('close', (hadError) => { clearTimeout(connectTimeout); console.debug('[IPC] meshcore:tcp socket closed', hadError ? '(hadError)' : '(clean)'); - mainWindow?.webContents.send('meshcore:tcp-disconnected'); - if (meshcoreTcpSocket === socket) meshcoreTcpSocket = null; + // Only notify when this socket is still the active bridge. connect/disconnect clear the + // ref before destroy(), so superseded closes must not look like a live link drop + // (renderer reconnect is driven by this event — see #792). + if (meshcoreTcpSocket === socket) { + meshcoreTcpSocket = null; + mainWindow?.webContents.send('meshcore:tcp-disconnected'); + } }); socket.on('error', (err) => { clearTimeout(connectTimeout); @@ -6231,8 +6236,13 @@ ipcMain.handle('meshtastic:tcp-connect', (event, host: string, port: number) => socket.on('close', (hadError) => { clearTimeout(connectTimeout); console.debug('[IPC] meshtastic:tcp socket closed', hadError ? '(hadError)' : '(clean)'); - mainWindow?.webContents.send('meshtastic:tcp-disconnected'); - if (meshtasticTcpSocket === socket) meshtasticTcpSocket = null; + // Only notify when this socket is still the active bridge. connect/disconnect clear the + // ref before destroy(), so superseded closes must not look like a live link drop + // (renderer reconnect is driven by this event — see #792). + if (meshtasticTcpSocket === socket) { + meshtasticTcpSocket = null; + mainWindow?.webContents.send('meshtastic:tcp-disconnected'); + } }); socket.on('error', (err) => { clearTimeout(connectTimeout); From 2e3f232ec935bb7f00edfdbf037673a3b008d639 Mon Sep 17 00:00:00 2001 From: Joe WB3IHY Date: Tue, 4 Aug 2026 23:04:40 -0400 Subject: [PATCH 5/5] docs: correct stale BLE-only wording on NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS Doc comment still said 'BLE reconnect' after the budget was applied to every transport in this PR. --- src/renderer/lib/timeConstants.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/renderer/lib/timeConstants.ts b/src/renderer/lib/timeConstants.ts index 4a0c99dfb..fec584573 100644 --- a/src/renderer/lib/timeConstants.ts +++ b/src/renderer/lib/timeConstants.ts @@ -211,8 +211,10 @@ export const MESHTASTIC_GET_METADATA_AFTER_CONFIGURE_RETRY_MS = 8_000; export const MESHTASTIC_BLE_CONFIGURE_TIMEOUT_MS = 30 * MS_PER_SECOND; /** - * Hard ceiling for one LoRa BLE reconnect open+handshake attempt (Meshtastic + MeshCore). - * Covers darwin dual createBleConnection attempts (~45–50s) + configure/attach margin so + * Hard ceiling for one LoRa reconnect open+configure/attach attempt (Meshtastic + MeshCore), + * applied to every transport (name is historical — BLE was the only transport with a deadline + * at all until TCP/serial/HTTP reconnects were found hanging indefinitely with none). For BLE, + * covers darwin dual createBleConnection attempts (~45–50s) + configure/attach margin so * deferred Noble disconnect flush always runs instead of stalling retries at edge of range. */ export const NOBLE_BLE_RECONNECT_ATTEMPT_BUDGET_MS =