From 536eca8f3d3f7d20025414c54ebcf2df6466c5fd Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Thu, 22 Oct 2026 13:28:43 -0400 Subject: [PATCH] fix(sse): retry an EventSource connect attempt that never opens An EventSource whose request never gets response headers stays in CONNECTING without firing open or error, so the backoff never runs and the staleness watchdog (which only judges a 'connected' stream) cannot see it. Bound each attempt with a 15s connect timeout: if open has not fired, close the attempt and retry through the same path onerror uses. The timer is cleared on open, on failure and at the top of every connectSSE(), and both the timeout and the failure handler ignore an EventSource that has since been replaced. --- src/web/public/app.js | 85 ++++++++---- src/web/public/constants.js | 10 ++ test/sse-connect-timeout.test.ts | 224 +++++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+), 26 deletions(-) create mode 100644 test/sse-connect-timeout.test.ts diff --git a/src/web/public/app.js b/src/web/public/app.js index 62716497..0471bb4d 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -988,6 +988,12 @@ class CodemanApp { this._sseLastMessageAt = 0; this._sseStaleTimeoutMs = window.CodemanSseStale?.TIMEOUT_MS ?? 45000; this._sseStaleWatchdog = null; + // Connect timeout: the watchdog only judges a 'connected' stream, and an + // attempt whose request hangs never opens AND never errors. Each attempt + // is bounded by this timer (armed in connectSSE, cleared on open, on + // failure and at the top of every connectSSE so timers never stack). + this._sseConnectTimeout = null; + this._sseConnectTimeoutMs = window.CodemanSseStale?.CONNECT_TIMEOUT_MS ?? 15000; // Connection-loss UI (banner + full-screen overlay). The decision itself is // pure and lives in constants.js (computeConnectionLossUi); these are just @@ -2057,6 +2063,10 @@ class CodemanApp { } connectSSE() { + // First, before the offline early return: any path into connectSSE + // supersedes the previous attempt, and its deadline must go with it. + this._clearTimer('_sseConnectTimeout'); + // Check if browser is offline if (!navigator.onLine) { this.setConnectionStatus('offline'); @@ -2100,7 +2110,21 @@ class CodemanApp { const _sseParams = new URLSearchParams({ clientId: this._clientId }); const _sseFilterId = this._sseFilterSessionId(this.activeSessionId); if (_sseFilterId) _sseParams.set('sessions', _sseFilterId); - this.eventSource = new EventSource(CodemanBase.url(`/api/events?${_sseParams.toString()}`)); + const eventSource = new EventSource(CodemanBase.url(`/api/events?${_sseParams.toString()}`)); + this.eventSource = eventSource; + + // Bound the attempt. A request that never gets response headers leaves + // the EventSource in CONNECTING with no `open` and no `error`, so without + // this the page waits forever. readyState OPEN covers the narrow race + // where the connection opened but its `open` callback has not run yet. + const connectTimeout = setTimeout(() => { + if (this._sseConnectTimeout !== connectTimeout) return; + this._sseConnectTimeout = null; + if (this.eventSource !== eventSource || eventSource.readyState === EventSource.OPEN) return; + console.warn(`[SSE] connection did not open within ${this._sseConnectTimeoutMs}ms, retrying`); + this._handleSseConnectionFailure(eventSource); + }, this._sseConnectTimeoutMs); + this._sseConnectTimeout = connectTimeout; // Store all event listeners for cleanup on reconnect. // @@ -2129,6 +2153,7 @@ class CodemanApp { }; this.eventSource.onopen = () => { + this._clearTimer('_sseConnectTimeout'); this.reconnectAttempts = 0; // Start the liveness clock here, not at the first frame: the watchdog // only ever fires while the status is 'connected', and this is the @@ -2136,31 +2161,7 @@ class CodemanApp { this._sseLastMessageAt = Date.now(); this.setConnectionStatus('connected'); }; - this.eventSource.onerror = () => { - this.reconnectAttempts++; - if (this.reconnectAttempts >= this.maxReconnectAttempts) { - this.setConnectionStatus('disconnected'); - } else { - this.setConnectionStatus('reconnecting'); - } - // Close the failed connection before scheduling reconnect - if (this.eventSource) { - this.eventSource.close(); - this.eventSource = null; - } - // Clear any existing reconnect timeout before setting new one (prevents orphaned timeouts) - this._clearTimer('sseReconnectTimeout'); - // Exponential backoff: 200ms, 500ms, 1s, 2s, 4s, ... up to 30s - // Fast first retry (200ms) for server-restart case (COM deploy), - // then ramp up for real network issues. - const delay = this.reconnectAttempts <= 1 ? 200 - : Math.min(500 * Math.pow(2, this.reconnectAttempts - 2), 30000); - // Feeds the "Retrying in Ns" countdown. With a 30s cap on the backoff, a - // silent wait that long is indistinguishable from a hung app. - this._nextSseRetryAt = Date.now() + delay; - this._updateConnectionLossUi(); - this.sseReconnectTimeout = setTimeout(() => this.connectSSE(), delay); - }; + this.eventSource.onerror = () => this._handleSseConnectionFailure(eventSource); // Create stable handler wrappers once (reused across reconnects so // removeEventListener always matches the original reference) @@ -2323,6 +2324,38 @@ class CodemanApp { this._sseStaleWatchdog = setInterval(() => this._checkSseStale(), 5000); } + /** + * Close a failed SSE attempt (onerror, or the connect timeout) and schedule + * the normal exponential-backoff retry. + */ + _handleSseConnectionFailure(eventSource) { + // A late error or timeout from an EventSource that has since been + // replaced must not close or reschedule the connection that replaced it. + if (this.eventSource !== eventSource) return; + this._clearTimer('_sseConnectTimeout'); + this.reconnectAttempts++; + if (this.reconnectAttempts >= this.maxReconnectAttempts) { + this.setConnectionStatus('disconnected'); + } else { + this.setConnectionStatus('reconnecting'); + } + // Close the failed connection before scheduling reconnect + this.eventSource = null; + eventSource.close(); + // Clear any existing reconnect timeout before setting new one (prevents orphaned timeouts) + this._clearTimer('sseReconnectTimeout'); + // Exponential backoff: 200ms, 500ms, 1s, 2s, 4s, ... up to 30s + // Fast first retry (200ms) for server-restart case (COM deploy), + // then ramp up for real network issues. + const delay = this.reconnectAttempts <= 1 ? 200 + : Math.min(500 * Math.pow(2, this.reconnectAttempts - 2), 30000); + // Feeds the "Retrying in Ns" countdown. With a 30s cap on the backoff, a + // silent wait that long is indistinguishable from a hung app. + this._nextSseRetryAt = Date.now() + delay; + this._updateConnectionLossUi(); + this.sseReconnectTimeout = setTimeout(() => this.connectSSE(), delay); + } + /** * Force a reconnect if the SSE stream has gone quiet while still claiming to * be connected. Called by the 5s watchdog and on tab-visible. diff --git a/src/web/public/constants.js b/src/web/public/constants.js index cd44962f..abb8381e 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -783,6 +783,15 @@ function formatPromptCacheStatus(cache, now, statusLine) { // Pure: no DOM, no timers, no side effects. `now` is passed in. const SSE_STALE_TIMEOUT_MS = 45000; // three missed 15s heartbeats +// The watchdog above cannot see a connect attempt that never opens: an +// EventSource whose request gets no response headers (a resumed phone on a +// VPN, a proxy that accepted the socket and went silent) stays CONNECTING +// without firing `open` or `error`, and its status is never 'connected'. +// connectSSE() bounds each attempt by this much and then retries it through +// the normal backoff. Generous next to a healthy open (well under a second) +// so a slow link is not cut off mid-handshake. +const SSE_CONNECT_TIMEOUT_MS = 15000; + function computeSseStale(input) { const { lastMessageAt = null, @@ -1500,6 +1509,7 @@ if (typeof window !== 'undefined') { window.CodemanSseStale = { compute: computeSseStale, TIMEOUT_MS: SSE_STALE_TIMEOUT_MS, + CONNECT_TIMEOUT_MS: SSE_CONNECT_TIMEOUT_MS, }; window.CodemanPromptCache = { format: formatPromptCacheStatus, diff --git a/test/sse-connect-timeout.test.ts b/test/sse-connect-timeout.test.ts new file mode 100644 index 00000000..941292c7 --- /dev/null +++ b/test/sse-connect-timeout.test.ts @@ -0,0 +1,224 @@ +/** + * @fileoverview SSE connect timeout: an EventSource whose first request never + * gets response headers sits in CONNECTING forever without firing `open` or + * `error` (a resumed phone on a VPN, a proxy that accepted the TCP connection + * and went silent). The staleness watchdog (test/sse-staleness.test.ts) cannot + * see it, because it only judges a stream that is already 'connected'. So + * `connectSSE()` bounds every attempt: if `open` has not fired within + * `_sseConnectTimeoutMs`, the attempt is closed and retried through the same + * backoff path `onerror` uses. + * + * Real code: constants.js + app.js in a `vm` context with a fake EventSource + * and vitest fake timers. Port: N/A. + */ +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +class FakeEventSource { + static CONNECTING = 0; + static OPEN = 1; + static CLOSED = 2; + static all: FakeEventSource[] = []; + readyState = FakeEventSource.CONNECTING; + closed = false; + onopen: (() => void) | null = null; + onerror: (() => void) | null = null; + constructor(public url: string) { + FakeEventSource.all.push(this); + } + addEventListener() {} + removeEventListener() {} + close() { + this.closed = true; + this.readyState = FakeEventSource.CLOSED; + } + open() { + this.readyState = FakeEventSource.OPEN; + this.onopen?.(); + } + fail() { + this.onerror?.(); + } +} + +const navigatorStub = { onLine: true }; +const windowStub: Record = { addEventListener: vi.fn(), removeEventListener: vi.fn() }; +const read = (f: string) => readFileSync(resolve(import.meta.dirname, `../src/web/public/${f}`), 'utf8'); +const context = vm.createContext({ + console: { ...console, log: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn() }, + performance: { now: () => 0 }, + setInterval: () => 0, + clearInterval: () => {}, + // Resolved at call time so vitest's fake timers (installed per test) apply. + setTimeout: (fn: () => void, ms?: number) => globalThis.setTimeout(fn, ms), + clearTimeout: (id: ReturnType) => globalThis.clearTimeout(id), + HTMLCanvasElement: class {}, + EventSource: FakeEventSource, + URLSearchParams, + navigator: navigatorStub, + location: { protocol: 'http:', host: 'codeman.test', pathname: '/', search: '', hash: '' }, + document: { + addEventListener: () => {}, + removeEventListener: () => {}, + documentElement: { dataset: {} }, + getElementById: () => null, + querySelector: () => null, + querySelectorAll: () => [], + }, + localStorage: { getItem: () => null, setItem: () => {}, removeItem: () => {} }, + window: windowStub, + MobileDetection: { isTouchDevice: () => false, isHandheldDevice: () => false, getDeviceType: () => 'desktop' }, +}); +vm.runInContext(`${read('constants.js')}\n${read('app.js')}\nglobalThis.__CodemanApp = CodemanApp;`, context); +const CodemanApp = (context as unknown as { __CodemanApp: { prototype: object } }).__CodemanApp; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type App = Record; + +function makeApp(): App { + const app = Object.create(CodemanApp.prototype) as App; + app._clientId = 'client-1'; + app.activeSessionId = null; + app.reconnectAttempts = 0; + app.maxReconnectAttempts = 10; + app.sseReconnectTimeout = null; + app._sseListenerCleanup = null; + app._sseStaleWatchdog = null; + app._sseLastMessageAt = 0; + app._nextSseRetryAt = null; + app.eventSource = null; + // What the constructor seeds (the policy constant lives in constants.js). + app._sseConnectTimeout = null; + app._sseConnectTimeoutMs = (windowStub.CodemanSseStale as { CONNECT_TIMEOUT_MS?: number }).CONNECT_TIMEOUT_MS; + app.statuses = [] as string[]; + app.setConnectionStatus = vi.fn((s: string) => app.statuses.push(s)); + app._updateConnectionLossUi = vi.fn(); + return app; +} + +const TIMEOUT = 15000; + +describe('SSE connect timeout', () => { + beforeEach(() => { + vi.useFakeTimers(); + FakeEventSource.all = []; + navigatorStub.onLine = true; + }); + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + }); + + it('defaults to 15s', () => { + expect((windowStub.CodemanSseStale as { CONNECT_TIMEOUT_MS?: number }).CONNECT_TIMEOUT_MS).toBe(TIMEOUT); + const app = makeApp(); + app.connectSSE(); + vi.advanceTimersByTime(TIMEOUT - 1); + expect(FakeEventSource.all[0].closed).toBe(false); + vi.advanceTimersByTime(1); + expect(FakeEventSource.all[0].closed).toBe(true); + }); + + it('closes an attempt that never opens and retries it through the normal backoff', () => { + const app = makeApp(); + app.connectSSE(); + const first = FakeEventSource.all[0]; + vi.advanceTimersByTime(TIMEOUT); + + expect(first.closed).toBe(true); + expect(app.eventSource).toBe(null); + expect(app.reconnectAttempts).toBe(1); + expect(app.statuses.at(-1)).toBe('reconnecting'); + // Feeds the connection-loss UI countdown, same as onerror. + expect(app._nextSseRetryAt).toBeGreaterThan(0); + expect(app._updateConnectionLossUi).toHaveBeenCalled(); + + // First retry uses the fast 200ms backoff step. + vi.advanceTimersByTime(199); + expect(FakeEventSource.all).toHaveLength(1); + vi.advanceTimersByTime(1); + expect(FakeEventSource.all).toHaveLength(2); + expect(app.eventSource).toBe(FakeEventSource.all[1]); + }); + + it('keeps retrying each hung attempt, with backoff growing', () => { + const app = makeApp(); + app.connectSSE(); + vi.advanceTimersByTime(TIMEOUT + 200); // attempt 1 times out, attempt 2 starts + vi.advanceTimersByTime(TIMEOUT + 500); // attempt 2 times out, attempt 3 starts + expect(FakeEventSource.all).toHaveLength(3); + expect(app.reconnectAttempts).toBe(2); + expect(FakeEventSource.all.slice(0, 2).every((es) => es.closed)).toBe(true); + }); + + it('an attempt that opens is never timed out', () => { + const app = makeApp(); + app.connectSSE(); + vi.advanceTimersByTime(1000); + FakeEventSource.all[0].open(); + vi.advanceTimersByTime(TIMEOUT * 4); + expect(FakeEventSource.all).toHaveLength(1); + expect(FakeEventSource.all[0].closed).toBe(false); + expect(app.reconnectAttempts).toBe(0); + }); + + it('an attempt that errors is retried once, not again by its timeout', () => { + const app = makeApp(); + app.connectSSE(); + FakeEventSource.all[0].fail(); + expect(app.reconnectAttempts).toBe(1); + vi.advanceTimersByTime(200); + expect(FakeEventSource.all).toHaveLength(2); + FakeEventSource.all[1].open(); + vi.advanceTimersByTime(TIMEOUT * 4); + expect(FakeEventSource.all).toHaveLength(2); + expect(FakeEventSource.all[1].closed).toBe(false); + expect(app.reconnectAttempts).toBe(0); + }); + + it('a reconnect replaces the pending timer instead of stacking a second one', () => { + const app = makeApp(); + app.connectSSE(); + vi.advanceTimersByTime(10000); + app.connectSSE(); // e.g. the user pressed Retry + expect(FakeEventSource.all).toHaveLength(2); + // The first attempt's deadline passes: nothing may happen to the second. + vi.advanceTimersByTime(TIMEOUT - 10000); + expect(FakeEventSource.all[1].closed).toBe(false); + expect(app.reconnectAttempts).toBe(0); + // The second attempt's own deadline still applies. + vi.advanceTimersByTime(10000); + expect(FakeEventSource.all[1].closed).toBe(true); + expect(app.reconnectAttempts).toBe(1); + }); + + it('going offline cancels the pending timer (the online event reconnects)', () => { + const app = makeApp(); + app.connectSSE(); + navigatorStub.onLine = false; + app.connectSSE(); + expect(app.statuses.at(-1)).toBe('offline'); + vi.advanceTimersByTime(TIMEOUT * 4); + expect(app.reconnectAttempts).toBe(0); + expect(app.statuses.at(-1)).toBe('offline'); + }); + + it('does not retry a connection that opened but whose open callback has not run yet', () => { + const app = makeApp(); + app.connectSSE(); + FakeEventSource.all[0].readyState = FakeEventSource.OPEN; + vi.advanceTimersByTime(TIMEOUT); + expect(FakeEventSource.all[0].closed).toBe(false); + expect(app.reconnectAttempts).toBe(0); + }); + + it('honours a shrunk _sseConnectTimeoutMs (what a browser test sets)', () => { + const app = makeApp(); + app._sseConnectTimeoutMs = 500; + app.connectSSE(); + vi.advanceTimersByTime(500); + expect(FakeEventSource.all[0].closed).toBe(true); + }); +});