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); + }); +});