diff --git a/src/main/meshcore-mqtt-adapter.test.ts b/src/main/meshcore-mqtt-adapter.test.ts index 0a53e989f..9b1b41995 100644 --- a/src/main/meshcore-mqtt-adapter.test.ts +++ b/src/main/meshcore-mqtt-adapter.test.ts @@ -141,6 +141,77 @@ describe('MeshcoreMqttAdapter — PING logging', () => { }); }); +describe('MeshcoreMqttAdapter — stale client isolation', () => { + let adapter: MeshcoreMqttAdapter; + + interface AdapterPacketPrivate { + lastPacketReceivedAt: number; + } + + const makeClient = () => ({ + on: vi.fn(), + end: vi.fn(), + removeAllListeners: vi.fn(), + connected: false, + publish: vi.fn(), + subscribe: vi.fn(), + reschedulePing: vi.fn(), + options: {}, + stream: {}, + }); + + const lastHandler = ( + client: { on: ReturnType }, + name: string, + ): ((packet: { cmd: string }) => void) => { + const hits = client.on.mock.calls.filter((c: unknown[]) => c[0] === name); + return hits[hits.length - 1]?.[1] as (packet: { cmd: string }) => void; + }; + + beforeEach(async () => { + const mqtt = await import('mqtt'); + vi.mocked(mqtt.connect).mockClear(); + adapter = new MeshcoreMqttAdapter(); + adapter.on('error', () => {}); + }); + + afterEach(() => { + adapter.disconnect(); + vi.restoreAllMocks(); + }); + + it('ignores packet events from a client that is no longer this.client', async () => { + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); + const mqttMod = await import('mqtt'); + const first = makeClient(); + const second = makeClient(); + vi.mocked(mqttMod.connect) + .mockImplementationOnce(() => first as never) + .mockImplementationOnce(() => second as never); + + adapter.connect({ ...BASE_SETTINGS }); + // Reconnect: connect() force-ends `first` and installs `second` as this.client. + adapter.connect({ ...BASE_SETTINGS }); + + const priv = adapter as unknown as AdapterPacketPrivate; + priv.lastPacketReceivedAt = 0; + + // Stale `first` client emits after it was replaced — must be ignored entirely. + lastHandler(first, 'packetreceive')({ cmd: 'pingresp' }); + lastHandler(first, 'packetsend')({ cmd: 'pingreq' }); + expect(priv.lastPacketReceivedAt).toBe(0); + expect(debugSpy.mock.calls.filter((c) => String(c[0]).includes('PINGRESP'))).toHaveLength(0); + expect(debugSpy.mock.calls.filter((c) => String(c[0]).includes('PINGREQ'))).toHaveLength(0); + + // Live `second` client still updates state and consumes the first-ping logs. + lastHandler(second, 'packetreceive')({ cmd: 'pingresp' }); + lastHandler(second, 'packetsend')({ cmd: 'pingreq' }); + expect(priv.lastPacketReceivedAt).toBeGreaterThan(0); + expect(debugSpy.mock.calls.filter((c) => String(c[0]).includes('PINGRESP'))).toHaveLength(1); + expect(debugSpy.mock.calls.filter((c) => String(c[0]).includes('PINGREQ'))).toHaveLength(1); + }); +}); + describe('MeshcoreMqttAdapter — clientId', () => { let adapter: MeshcoreMqttAdapter; diff --git a/src/main/meshcore-mqtt-adapter.ts b/src/main/meshcore-mqtt-adapter.ts index 3afa7fa09..93d127609 100644 --- a/src/main/meshcore-mqtt-adapter.ts +++ b/src/main/meshcore-mqtt-adapter.ts @@ -316,6 +316,10 @@ export class MeshcoreMqttAdapter extends EventEmitter { this.setStatus('connecting'); this.connectAbortByWatchdog = false; this.client = mqtt.connect(connectOpts); + // Capture this session's client so listeners from an already-replaced (stale) client + // — mqtt.js can still emit after end() — cannot touch lastPacketReceivedAt or consume + // the new session's first-ping logs. + const sessionClient = this.client; this.client.on('error', (err) => { this.clearConnectTimers(); console.error( @@ -406,7 +410,8 @@ export class MeshcoreMqttAdapter extends EventEmitter { // Schedule proactive token refresh this.scheduleTokenRefresh(); }); - this.client.on('packetsend', (packet) => { + sessionClient.on('packetsend', (packet) => { + if (this.client !== sessionClient) return; if (packet.cmd === 'pingreq' && !this.pingReqLogged) { this.pingReqLogged = true; console.debug( @@ -415,7 +420,8 @@ export class MeshcoreMqttAdapter extends EventEmitter { ); } }); - this.client.on('packetreceive', (packet) => { + sessionClient.on('packetreceive', (packet) => { + if (this.client !== sessionClient) return; this.lastPacketReceivedAt = Date.now(); if (packet.cmd === 'pingresp' && !this.pingRespLogged) { this.pingRespLogged = true; diff --git a/src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.ts b/src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.ts index 0bc1e5147..35e452a15 100644 --- a/src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.ts +++ b/src/renderer/lib/meshtastic/meshtasticRuntimeWireEffects.ts @@ -622,11 +622,19 @@ export function attachMeshtasticRuntimeWireEffects( }); unsubscribesRef.current.push(unsub2); - const maybeRequestNodeInfoForNode = (from: number): void => { + const maybeRequestNodeInfoForNode = ( + from: number, + opts?: { ignoreDisplayIdentity?: boolean }, + ): void => { if (from === 0 || from === myNodeNumRef.current) return; if (isConfiguringRef.current) return; - const existing = getIdentityNode(meshtasticIdentityIdRef.current, from); - if (existing && !meshtasticNodeLacksDisplayIdentity(existing, from)) return; + // Missing-recipient-key recovery must refresh even nodes that already have a + // display name (we know who they are, we just lack a usable public key), so it + // opts out of the display-identity short-circuit while keeping the rate limit. + if (!opts?.ignoreDisplayIdentity) { + const existing = getIdentityNode(meshtasticIdentityIdRef.current, from); + if (existing && !meshtasticNodeLacksDisplayIdentity(existing, from)) return; + } const now = Date.now(); const last = lastNodeInfoRequestAtRef.current.get(from) ?? 0; if (now - last < REQUEST_NODEINFO_MIN_INTERVAL_MS) return; @@ -834,7 +842,9 @@ export function attachMeshtasticRuntimeWireEffects( myNodeNum: myNodeNumRef.current, identityId: meshtasticIdentityIdRef.current, tempIdToWirePacketId: ackMeshPacketIdByTempIdRef.current, - onMissingRecipientKey: maybeRequestNodeInfoForNode, + onMissingRecipientKey: (recipientNodeNum) => { + maybeRequestNodeInfoForNode(recipientNodeNum, { ignoreDisplayIdentity: true }); + }, }); }; @@ -843,7 +853,9 @@ export function attachMeshtasticRuntimeWireEffects( myNodeNum: myNodeNumRef.current, identityId: meshtasticIdentityIdRef.current, tempIdToWirePacketId: ackMeshPacketIdByTempIdRef.current, - onMissingRecipientKey: maybeRequestNodeInfoForNode, + onMissingRecipientKey: (recipientNodeNum) => { + maybeRequestNodeInfoForNode(recipientNodeNum, { ignoreDisplayIdentity: true }); + }, }); if (!uiApplied) { const parsed = reason as { id?: number; packetId?: number; error?: number }; diff --git a/src/renderer/lib/meshtastic/meshtasticSdkRoutingErrorLog.ts b/src/renderer/lib/meshtastic/meshtasticSdkRoutingErrorLog.ts index 4b8f12b70..17bce8a3c 100644 --- a/src/renderer/lib/meshtastic/meshtasticSdkRoutingErrorLog.ts +++ b/src/renderer/lib/meshtastic/meshtasticSdkRoutingErrorLog.ts @@ -4,6 +4,7 @@ import { resolveMeshtasticOutboundStoreKey } from '@/renderer/lib/sessions/mesht import { messageRecordsToChatMessages } from '@/renderer/lib/storeRecordAdapters'; import type { ChatMessage } from '@/renderer/lib/types'; import { updateMessageStatus, useMessageStore } from '@/renderer/stores/messageStore'; +import { isMeshtasticBroadcastNodeNum } from '@/shared/nodeNameUtils'; import { meshtasticRoutingErrorName } from './meshtasticApplyErrorMessage'; import { @@ -202,7 +203,13 @@ export function applyMeshtasticOutboundRoutingError( const storeMessageId = resolveStoreMessageId(target, parsed.packetId); updateMessageStatus(identityId, storeMessageId, 'failed', errorText); // Missing recipient public key: fetch the recipient's NODEINFO so a retry can succeed. - if (isMeshtasticMissingRecipientKeyError(parsed.errorName) && target.to != null) { + // Only for real DM recipients — never the broadcast address (a PKI NAK there is not + // a per-node key gap, and 0xffffffff has no NODEINFO to fetch). + if ( + isMeshtasticMissingRecipientKeyError(parsed.errorName) && + target.to != null && + !isMeshtasticBroadcastNodeNum(target.to) + ) { ctx.onMissingRecipientKey?.(target.to); } // The DB row may still hold the optimistic temp packet id (device never acked, diff --git a/src/renderer/stores/reticulumPeerStore.test.ts b/src/renderer/stores/reticulumPeerStore.test.ts index 87cb689d3..2e2bc46a8 100644 --- a/src/renderer/stores/reticulumPeerStore.test.ts +++ b/src/renderer/stores/reticulumPeerStore.test.ts @@ -1101,6 +1101,56 @@ describe('reticulumPeerStore', () => { }); }); + const stubRefreshWindow = (): void => { + vi.stubGlobal('window', { + electronAPI: { + reticulum: { + proxyGet: vi.fn((path: string) => { + if (path === '/api/v1/contacts') return Promise.resolve({ contacts: [] }); + if (path === '/api/v1/peers' || path.startsWith('/api/v1/peers?')) { + return Promise.resolve({ peers: [{ destination_hash: 'bb', hops: 3 }] }); + } + if (path === '/api/v1/nomadnetwork/nodes') return Promise.resolve({ nodes: [] }); + return Promise.resolve({}); + }), + }, + db: { getReticulumDestinations: vi.fn().mockResolvedValue([]) }, + }, + }); + }; + + it('does not log a full-refresh debug line when refresh completes under 2s', async () => { + stubRefreshWindow(); + // started and elapsed both read the same clock value → elapsed 0ms. + vi.spyOn(performance, 'now').mockReturnValue(1000); + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); + + await refreshReticulumPeersFromSidecar({ forceRefresh: true }); + + expect(useReticulumPeerStore.getState().peers.get('bb')?.hops).toBe(3); + expect(debugSpy.mock.calls.filter((c) => String(c[0]).includes('full refresh'))).toHaveLength( + 0, + ); + debugSpy.mockRestore(); + }); + + it('logs a full-refresh debug line when refresh exceeds the 2s threshold', async () => { + stubRefreshWindow(); + // First now() call seeds `started` at 0; all later calls (incl. elapsed calc) → 5000ms. + vi.spyOn(performance, 'now').mockReturnValueOnce(0).mockReturnValue(5000); + const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); + + await refreshReticulumPeersFromSidecar({ forceRefresh: true }); + + expect(useReticulumPeerStore.getState().peers.get('bb')?.hops).toBe(3); + const fullRefreshLogs = debugSpy.mock.calls.filter((c) => + String(c[0]).includes('full refresh'), + ); + expect(fullRefreshLogs).toHaveLength(1); + expect(String(fullRefreshLogs[0][0])).toContain('5000ms'); + debugSpy.mockRestore(); + }); + it('applyReticulumAnnounceReceivedOptimistic inserts a peer before path-table refresh', () => { applyReticulumAnnounceReceivedOptimistic({ destination_hash: 'AaBbCcDdEeFf00112233445566778899',