Skip to content

Commit f98b6d0

Browse files
committed
fix(connection): prevent resource conflict ping-pong loop
Three fixes to prevent a reconnection loop when the same XMPP resource is bound by multiple connections: - Validate stored resource format in getResource() and regenerate stale bare values (e.g. "web" without random suffix) that predate the resource randomization system - Make connect() a no-op when already connecting/connected/reconnecting to prevent double-connect races that create two sockets binding the same resource - Skip stale client disconnect recovery (SOCKET_DIED) when the state machine is in a terminal state (conflict, authFailed) to break the conflict -> stale disconnect -> reconnect -> conflict cycle Also simplifies the state machine: CONNECT from terminal states now transitions directly to connecting (was idle, requiring two events).
1 parent 8b2aed4 commit f98b6d0

7 files changed

Lines changed: 430 additions & 26 deletions

File tree

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
2+
import { generateResource, isValidResource } from './xmppResource'
3+
4+
// Mock isTauri — must be before importing getResource
5+
vi.mock('./tauri', () => ({
6+
isTauri: vi.fn(() => false),
7+
}))
8+
9+
import { isTauri } from './tauri'
10+
11+
function createStorageMock(): Storage {
12+
const store: Record<string, string> = {}
13+
return {
14+
getItem: vi.fn((key: string) => store[key] ?? null),
15+
setItem: vi.fn((key: string, value: string) => { store[key] = value }),
16+
removeItem: vi.fn((key: string) => { delete store[key] }),
17+
clear: vi.fn(() => { Object.keys(store).forEach(k => delete store[k]) }),
18+
get length() { return Object.keys(store).length },
19+
key: vi.fn((index: number) => Object.keys(store)[index] ?? null),
20+
}
21+
}
22+
23+
describe('xmppResource', () => {
24+
describe('generateResource', () => {
25+
it('should generate a resource with the given prefix and 6-char suffix', () => {
26+
const resource = generateResource('web')
27+
expect(resource).toMatch(/^web-[a-z0-9]{6}$/)
28+
})
29+
30+
it('should generate unique resources', () => {
31+
const resources = new Set(Array.from({ length: 20 }, () => generateResource('web')))
32+
expect(resources.size).toBeGreaterThan(1)
33+
})
34+
35+
it('should use the provided prefix', () => {
36+
expect(generateResource('desktop')).toMatch(/^desktop-/)
37+
expect(generateResource('mobile')).toMatch(/^mobile-/)
38+
})
39+
})
40+
41+
describe('isValidResource', () => {
42+
it('should accept valid web resources', () => {
43+
expect(isValidResource('web-abc123')).toBe(true)
44+
expect(isValidResource('web-000000')).toBe(true)
45+
expect(isValidResource('web-zzzzzz')).toBe(true)
46+
})
47+
48+
it('should accept valid desktop resources', () => {
49+
expect(isValidResource('desktop-abc123')).toBe(true)
50+
})
51+
52+
it('should reject bare prefix without suffix', () => {
53+
expect(isValidResource('web')).toBe(false)
54+
expect(isValidResource('desktop')).toBe(false)
55+
})
56+
57+
it('should reject resources with wrong suffix length', () => {
58+
expect(isValidResource('web-abc')).toBe(false)
59+
expect(isValidResource('web-abc1234')).toBe(false)
60+
})
61+
62+
it('should reject resources with invalid characters in suffix', () => {
63+
expect(isValidResource('web-ABC123')).toBe(false)
64+
expect(isValidResource('web-abc!23')).toBe(false)
65+
})
66+
67+
it('should reject unknown prefixes', () => {
68+
expect(isValidResource('mobile-abc123')).toBe(false)
69+
expect(isValidResource('foo-abc123')).toBe(false)
70+
})
71+
72+
it('should reject empty string', () => {
73+
expect(isValidResource('')).toBe(false)
74+
})
75+
})
76+
77+
describe('getResource', () => {
78+
let mockSessionStorage: Storage
79+
let mockLocalStorage: Storage
80+
let originalSessionStorage: Storage
81+
let originalLocalStorage: Storage
82+
83+
beforeEach(async () => {
84+
mockSessionStorage = createStorageMock()
85+
mockLocalStorage = createStorageMock()
86+
originalSessionStorage = globalThis.sessionStorage
87+
originalLocalStorage = globalThis.localStorage
88+
vi.stubGlobal('sessionStorage', mockSessionStorage)
89+
vi.stubGlobal('localStorage', mockLocalStorage)
90+
91+
// Re-import to pick up mocked storage
92+
vi.resetModules()
93+
})
94+
95+
afterEach(() => {
96+
vi.stubGlobal('sessionStorage', originalSessionStorage)
97+
vi.stubGlobal('localStorage', originalLocalStorage)
98+
vi.restoreAllMocks()
99+
})
100+
101+
async function loadGetResource() {
102+
const mod = await import('./xmppResource')
103+
return mod.getResource
104+
}
105+
106+
it('should generate a new web resource when sessionStorage is empty', async () => {
107+
vi.mocked(isTauri).mockReturnValue(false)
108+
const getRes = await loadGetResource()
109+
const resource = getRes()
110+
expect(resource).toMatch(/^web-[a-z0-9]{6}$/)
111+
expect(mockSessionStorage.setItem).toHaveBeenCalledWith('xmpp-resource', resource)
112+
})
113+
114+
it('should return existing valid web resource from sessionStorage', async () => {
115+
vi.mocked(isTauri).mockReturnValue(false)
116+
mockSessionStorage.setItem('xmpp-resource', 'web-abc123')
117+
const getRes = await loadGetResource()
118+
expect(getRes()).toBe('web-abc123')
119+
})
120+
121+
it('should regenerate when sessionStorage has bare "web" (stale value)', async () => {
122+
vi.mocked(isTauri).mockReturnValue(false)
123+
mockSessionStorage.setItem('xmpp-resource', 'web')
124+
const getRes = await loadGetResource()
125+
const resource = getRes()
126+
expect(resource).toMatch(/^web-[a-z0-9]{6}$/)
127+
expect(resource).not.toBe('web')
128+
})
129+
130+
it('should generate a new desktop resource when localStorage is empty', async () => {
131+
vi.mocked(isTauri).mockReturnValue(true)
132+
const getRes = await loadGetResource()
133+
const resource = getRes()
134+
expect(resource).toMatch(/^desktop-[a-z0-9]{6}$/)
135+
expect(mockLocalStorage.setItem).toHaveBeenCalledWith('xmpp-resource', resource)
136+
})
137+
138+
it('should return existing valid desktop resource from localStorage', async () => {
139+
vi.mocked(isTauri).mockReturnValue(true)
140+
mockLocalStorage.setItem('xmpp-resource', 'desktop-xyz789')
141+
const getRes = await loadGetResource()
142+
expect(getRes()).toBe('desktop-xyz789')
143+
})
144+
145+
it('should regenerate when localStorage has bare "desktop" (stale value)', async () => {
146+
vi.mocked(isTauri).mockReturnValue(true)
147+
mockLocalStorage.setItem('xmpp-resource', 'desktop')
148+
const getRes = await loadGetResource()
149+
const resource = getRes()
150+
expect(resource).toMatch(/^desktop-[a-z0-9]{6}$/)
151+
expect(resource).not.toBe('desktop')
152+
})
153+
})
154+
})

apps/fluux/src/utils/xmppResource.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ const RESOURCE_KEY = 'xmpp-resource'
2121
* Generates a random resource identifier with the given prefix.
2222
* Format: "{prefix}-XXXXXX" where X is alphanumeric
2323
*/
24-
function generateResource(prefix: string): string {
24+
export function generateResource(prefix: string): string {
2525
const chars = 'abcdefghijklmnopqrstuvwxyz0123456789'
2626
let suffix = ''
2727
for (let i = 0; i < 6; i++) {
@@ -30,25 +30,36 @@ function generateResource(prefix: string): string {
3030
return `${prefix}-${suffix}`
3131
}
3232

33+
/**
34+
* Validates that a stored resource has the expected format: "{prefix}-XXXXXX".
35+
* Rejects bare prefixes (e.g. "web", "desktop") that predate the random suffix system.
36+
*/
37+
export function isValidResource(resource: string): boolean {
38+
return /^(web|desktop)-[a-z0-9]{6}$/.test(resource)
39+
}
40+
3341
/**
3442
* Gets the XMPP resource for the current client.
3543
*
3644
* - For Tauri: Returns a persistent random resource stored in localStorage
3745
* - For Web: Returns a unique-per-tab resource stored in sessionStorage
46+
*
47+
* If a stored resource has an invalid format (e.g. a bare "web" or "desktop"
48+
* from before the random suffix system), it is regenerated.
3849
*/
3950
export function getResource(): string {
4051
if (isTauri()) {
4152
// Desktop: Use persistent random resource (survives app restarts)
4253
let resource = localStorage.getItem(RESOURCE_KEY)
43-
if (!resource) {
54+
if (!resource || !isValidResource(resource)) {
4455
resource = generateResource('desktop')
4556
localStorage.setItem(RESOURCE_KEY, resource)
4657
}
4758
return resource
4859
} else {
4960
// Web: Use per-tab resource (survives page reload, unique per tab)
5061
let resource = sessionStorage.getItem(RESOURCE_KEY)
51-
if (!resource) {
62+
if (!resource || !isValidResource(resource)) {
5263
resource = generateResource('web')
5364
sessionStorage.setItem(RESOURCE_KEY, resource)
5465
}

packages/fluux-sdk/src/core/XMPPClient.test.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,9 +1490,10 @@ describe('XMPPClient', () => {
14901490
// Start health check — this will call waitForSmAck on the current client
14911491
const resultPromise = xmppClient.verifyConnectionHealth()
14921492

1493-
// Simulate reconnection: replace the xmpp client before the ack timeout fires.
1494-
// This is what happens when forceDestroyClient strips listeners on the old client
1495-
// and a new connection is established via SM resumption.
1493+
// Simulate reconnection: disconnect first, then reconnect with a new client.
1494+
// This is what happens when the connection drops and auto-reconnect fires.
1495+
await xmppClient.disconnect()
1496+
14961497
const newClient = createMockXmppClient()
14971498
newClient.streamManagement = {
14981499
id: 'sm-456',
@@ -1547,7 +1548,9 @@ describe('XMPPClient', () => {
15471548

15481549
const resultPromise = xmppClient.verifyConnection()
15491550

1550-
// Simulate reconnection mid-verify
1551+
// Simulate reconnection mid-verify: disconnect first, then reconnect
1552+
await xmppClient.disconnect()
1553+
15511554
const newClient = createMockXmppClient()
15521555
newClient.streamManagement = {
15531556
id: 'sm-456',

packages/fluux-sdk/src/core/connectionMachine.test.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,9 @@ describe('connectionMachine', () => {
6363
actor.send({ type: 'CONNECTION_ERROR', error: 'some error' })
6464
expect(actor.getSnapshot().context.lastError).toBe('some error')
6565

66-
// CONNECT from terminal should clear error
67-
actor.send({ type: 'CONNECT' })
68-
// Now in idle (after CONNECT from terminal)
66+
// CONNECT from terminal should clear error and go directly to connecting
6967
actor.send({ type: 'CONNECT' })
68+
expect(actor.getSnapshot().value).toBe('connecting')
7069
expect(actor.getSnapshot().context.lastError).toBeNull()
7170

7271
actor.stop()
@@ -605,7 +604,7 @@ describe('connectionMachine', () => {
605604
actor.send({ type: 'CONFLICT' })
606605

607606
actor.send({ type: 'CONNECT' })
608-
expect(actor.getSnapshot().value).toBe('idle')
607+
expect(actor.getSnapshot().value).toBe('connecting')
609608
actor.stop()
610609
})
611610
})
@@ -658,7 +657,7 @@ describe('connectionMachine', () => {
658657
expect(actor.getSnapshot().value).toEqual({ terminal: 'authFailed' })
659658

660659
actor.send({ type: 'CONNECT' })
661-
expect(actor.getSnapshot().value).toBe('idle')
660+
expect(actor.getSnapshot().value).toBe('connecting')
662661
expect(actor.getSnapshot().context.lastError).toBeNull()
663662
actor.stop()
664663
})
@@ -722,7 +721,7 @@ describe('connectionMachine', () => {
722721
actor.send({ type: 'CONNECTION_ERROR', error: 'fail' })
723722

724723
actor.send({ type: 'CONNECT' })
725-
expect(actor.getSnapshot().value).toBe('idle')
724+
expect(actor.getSnapshot().value).toBe('connecting')
726725
expect(actor.getSnapshot().context.lastError).toBeNull()
727726
actor.stop()
728727
})

packages/fluux-sdk/src/core/connectionMachine.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -575,8 +575,8 @@ export const connectionMachine = setup({
575575
initial: 'initialFailure',
576576
on: {
577577
CONNECT: {
578-
target: 'idle',
579-
actions: 'resetReconnectState',
578+
target: 'connecting',
579+
actions: ['resetReconnectState', 'clearError'],
580580
},
581581
},
582582
states: {

0 commit comments

Comments
 (0)