-
Notifications
You must be signed in to change notification settings - Fork 0
feat(transport): stable client connectionId + POST failure resilience #401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
d0a578e
46e6567
d59dcc9
1afc652
9bf203c
ac29be9
d4fd839
384227d
5ae9647
e430441
704dfc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ function createConfig(overrides?: Partial<SseConnectionConfig>): SseConnectionCo | |
| baseUrl: 'https://localhost:3100', | ||
| fetch: vi.fn().mockResolvedValue({ ok: true }), | ||
| createEventSource: (url: string) => new MockEventSource(url) as unknown as EventSource, | ||
| connectionId: 'cid-test', | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
@@ -93,7 +94,7 @@ describe('SseConnection', () => { | |
| conn.connect(); | ||
|
|
||
| expect(MockEventSource.instances).toHaveLength(1); | ||
| expect(lastES().url).toBe('https://localhost:3100/api/chat/events'); | ||
| expect(lastES().url).toBe('https://localhost:3100/api/chat/events?cid=cid-test'); | ||
| }); | ||
|
|
||
| it('becomes connected after welcome event', () => { | ||
|
|
@@ -804,9 +805,10 @@ describe('SseConnection', () => { | |
| // Force reconnect | ||
| conn.checkAndReconnect(true); | ||
|
|
||
| // URL should be clean — reconnect is handled via POST, not query param | ||
| // URL should include cid but NOT sessions (reconnect is handled via POST) | ||
| const newES = lastES(); | ||
| expect(newES.url).toBe('https://localhost:3100/api/chat/events'); | ||
| expect(newES.url).toContain('/api/chat/events?cid='); | ||
| expect(newES.url).not.toContain('sessions='); | ||
| }); | ||
|
|
||
| it('checkAndReconnect(false) is no-op when connected', () => { | ||
|
|
@@ -821,6 +823,80 @@ describe('SseConnection', () => { | |
| expect(MockEventSource.instances.length).toBe(count); | ||
| }); | ||
|
|
||
| // ─── Stable connectionId ──────────────────────────────────────────────── | ||
|
|
||
| describe('stable connectionId', () => { | ||
| it('includes client connectionId in EventSource URL', () => { | ||
| const conn = new SseConnection(createConfig({ connectionId: 'cid-my-tab' })); | ||
| conn.connect(); | ||
| expect(lastES().url).toBe('https://localhost:3100/api/chat/events?cid=cid-my-tab'); | ||
| }); | ||
|
|
||
| it('auto-generates connectionId when not provided', () => { | ||
| const conn = new SseConnection(createConfig({ connectionId: undefined })); | ||
| conn.connect(); | ||
| expect(lastES().url).toMatch(/\?cid=cid-[0-9a-f-]{36}$/); | ||
| }); | ||
|
|
||
| it('preserves connectionId when server echoes it back', () => { | ||
| const conn = new SseConnection(createConfig({ connectionId: 'cid-stable' })); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-stable', | ||
| }); | ||
| expect(conn.getConnectionId()).toBe('cid-stable'); | ||
| }); | ||
|
|
||
| it('falls back to server connectionId when it differs (old server)', () => { | ||
| const conn = new SseConnection(createConfig({ connectionId: 'cid-client' })); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'conn-server-assigned', | ||
| }); | ||
| expect(conn.getConnectionId()).toBe('conn-server-assigned'); | ||
| }); | ||
|
|
||
| it('uses same connectionId across reconnects', () => { | ||
| const conn = new SseConnection(createConfig({ connectionId: 'cid-stable' })); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-stable', | ||
| }); | ||
|
|
||
| conn.checkAndReconnect(true); | ||
|
|
||
| expect(lastES().url).toBe('https://localhost:3100/api/chat/events?cid=cid-stable'); | ||
| }); | ||
|
|
||
| it('sends connectionId in X-Connection-ID header on POST', () => { | ||
| const mockFetch = vi.fn().mockResolvedValue({ ok: true }); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch, connectionId: 'cid-hdr' })); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-hdr', | ||
| }); | ||
|
|
||
| conn.send({ type: 'send', prompt: 'hi' }); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| expect.objectContaining({ | ||
| headers: expect.objectContaining({ | ||
| 'X-Connection-ID': 'cid-hdr', | ||
| }), | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| // ─── Session tracking ────────────────────────────────────────────────── | ||
|
|
||
| it('trackSeq / getLastSeq / clearSession', () => { | ||
|
|
@@ -872,4 +948,126 @@ describe('SseConnection', () => { | |
|
|
||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| // ─── POST failure surfacing ─────────────────────────────────────────── | ||
|
|
||
| describe('POST failure surfacing', () => { | ||
| it('emits _send_failed on 500 response for send endpoint', async () => { | ||
| const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch })); | ||
| const listener = vi.fn(); | ||
| conn.onMessage(listener); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
|
|
||
| conn.send({ type: 'send', prompt: 'hello', clientMsgId: 'msg-1' }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| expect(listener).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: '_send_failed', | ||
| clientMsgId: 'msg-1', | ||
| willRetry: true, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('emits _send_failed on network error for send endpoint', async () => { | ||
| const mockFetch = vi.fn().mockRejectedValue(new Error('network error')); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch })); | ||
| const listener = vi.fn(); | ||
| conn.onMessage(listener); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
|
|
||
| conn.send({ type: 'send', prompt: 'hello', clientMsgId: 'msg-2' }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| expect(listener).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: '_send_failed', | ||
| clientMsgId: 'msg-2', | ||
| willRetry: true, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it('re-queues failed send for retry on reconnect', async () => { | ||
| let callCount = 0; | ||
| const mockFetch = vi.fn().mockImplementation(() => { | ||
| callCount++; | ||
| if (callCount === 1) return Promise.resolve({ ok: false, status: 500 }); | ||
| return Promise.resolve({ ok: true }); | ||
| }); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch })); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
|
|
||
| conn.send({ type: 'send', prompt: 'retry me', clientMsgId: 'r-1' }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| conn.checkAndReconnect(true); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('does not emit _send_failed for non-send endpoints', async () => { | ||
| const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch })); | ||
| const listener = vi.fn(); | ||
| conn.onMessage(listener); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
|
|
||
| conn.send({ type: 'stop', sessionId: 'sess-1' }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| expect(listener).not.toHaveBeenCalledWith(expect.objectContaining({ type: '_send_failed' })); | ||
| }); | ||
|
|
||
| it('emits _send_failed on 429 rate limit', async () => { | ||
| const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 429 }); | ||
| const conn = new SseConnection(createConfig({ fetch: mockFetch })); | ||
| const listener = vi.fn(); | ||
| conn.onMessage(listener); | ||
| conn.connect(); | ||
| lastES()._emit('welcome', { | ||
| type: 'welcome', | ||
| protocolVersion: 2, | ||
| connectionId: 'cid-test', | ||
| }); | ||
|
|
||
| conn.send({ type: 'send', prompt: 'hello', clientMsgId: 'msg-3' }); | ||
| await vi.runAllTimersAsync(); | ||
|
|
||
| expect(listener).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| type: '_send_failed', | ||
| willRetry: true, | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 missing_tests: The 'retries failed send after delay even without reconnect' test verifies the retry fires but doesn't check that _send_failed is NOT emitted on the successful retry (i.e., no spurious willRetry:false event). Adding an assertion like |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 unsafe_assumptions:
sessionStorage.getItem()andsetItem()can throwSecurityErrorin Safari private browsing, some Firefox configurations, and restrictive WebViews. SincegetOrCreateConnectionId()is called at module scope (during SSE config construction), an exception here would break the entire client import. Wrap in try-catch with a fallback to an in-memory UUID.[fixable]