diff --git a/src/web/public/app.js b/src/web/public/app.js index 522b542b..c6745af7 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -302,6 +302,14 @@ class CodemanApp { this._clientId = (typeof crypto !== 'undefined' && crypto.randomUUID) ? crypto.randomUUID() : 'c-' + Math.random().toString(36).slice(2) + Date.now().toString(36); + // Per-TAB nonce for the WS registry key (COD-137). _loadReliableState() + // later replaces _clientId with the browser-wide localStorage identity + // (shared by every tab/window of this profile), so the WS upgrade sends + // `clientId:nonce` instead — a same-tab reconnect still supersedes its own + // socket, but two tabs on one session coexist instead of evicting each + // other in a 4010 ping-pong. Input frames keep the bare clientId for seq + // dedup. + this._wsTabNonce = this._clientId; this.terminal = null; this.fitAddon = null; this.activeSessionId = null; @@ -436,10 +444,7 @@ class CodemanApp { this._wsSessionId = null; // Session ID the WS is connected to this._wsReady = false; // True when WS is open and ready for I/O this._wsState = 'disconnected'; // connecting | connected | reconnecting | fallback | disconnected - this._wsLastClose = null; // { code, reason, at } for transport diagnostics this._wsLastRecvAt = 0; // ms timestamp of the last frame received on the active WS - this._wsInputSendCount = 0; - this._httpFallbackSendCount = 0; // Terminal write batching with DEC 2026 sync support this.pendingWrites = []; @@ -1972,14 +1977,20 @@ class CodemanApp { */ _connectWs(sessionId) { this._disconnectWs(); + this._wsState = 'connecting'; + this._updateConnectionIndicator(); const proto = location.protocol === 'https:' ? 'wss:' : 'ws:'; - // Pass the stable per-browser clientId on the upgrade URL so the server's - // connection registry scopes the per-session limit by client (COD-137): a + // Pass a per-TAB identity on the upgrade URL so the server's connection + // registry scopes the per-session limit by connection (COD-137): a same-tab // reconnect supersedes its own socket instead of consuming a new slot and - // tripping a spurious 4008. Omitted if clientId is unavailable (server then - // treats the upgrade as anonymous — still admitted up to the limit). - const cidQuery = this._clientId ? `?cid=${encodeURIComponent(this._clientId)}` : ''; + // tripping a spurious 4008, while two tabs of the same browser (which share + // the localStorage clientId) each keep their own socket. The bare clientId + // still rides the input frames for seq dedup. Omitted if clientId is + // unavailable (server then treats the upgrade as anonymous — still admitted + // up to the limit). + const cid = this._clientId ? `${this._clientId}:${this._wsTabNonce}` : ''; + const cidQuery = cid ? `?cid=${encodeURIComponent(cid)}` : ''; const url = `${proto}//${location.host}/ws/sessions/${sessionId}/terminal${cidQuery}`; const ws = new WebSocket(url); this._ws = ws; @@ -1989,6 +2000,7 @@ class CodemanApp { // Only mark ready if this is still the intended session if (this._ws === ws) { this._wsReady = true; + this._wsState = 'connected'; this._wsReconnectAttempts = 0; this._updateConnectionIndicator(); // Send a typed resize over the fresh socket: syncs PTY dims after @@ -2092,7 +2104,11 @@ class CodemanApp { /** Close the active WebSocket connection (if any). */ _disconnectWs() { this._clearTimer('_wsReconnectTimer'); - this._wsReconnectAttempts = 0; + // Deliberately do NOT reset _wsReconnectAttempts here: _connectWs() calls + // this first, so a reset would restart the exponential backoff ladder at + // attempt 0 on every retry (≈0ms tight reconnect loop during an outage). + // ws.onopen zeroes the counter once a connection actually succeeds. + this._wsState = 'disconnected'; this._stopMobileResizeRetry(); if (this._ws) { this._ws.onclose = null; // Prevent re-entrant cleanup diff --git a/src/web/public/constants.js b/src/web/public/constants.js index be01ac6a..7b78afa1 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -126,38 +126,6 @@ function shouldAutoWrapTabs(input) { return scrollWidth > clientWidth + 1; } -// COD-122: Monitor "Tmux Sessions" row label policy. A row carries up to three -// identifiers — the tab name (user-facing L1 label), the tmux session name -// (e.g. "codeman-1fac7304"), and the agent (Claude/Codex) session id. The tab name -// must win as the primary label; the other two are secondary metadata shown only -// when known and not redundant with the primary, so the row degrades gracefully -// when identifiers are missing. -function resolveMonitorRowLabels(input) { - const clean = (v) => (typeof v === 'string' ? v.trim() : ''); - const tabName = clean(input && input.tabName); - const storedName = clean(input && input.storedName); - const muxName = clean(input && input.muxName); - const agentId = clean(input && input.agentSessionId); - const sessionId = clean(input && input.sessionId); - - // Tab name is the L1 label; fall back through the persisted mux name → tmux name - // → a generic placeholder so the primary is never blank. - const primary = tabName || storedName || muxName || 'session'; - - const secondaries = []; - // tmux name — skip when it's already serving as the primary fallback (no dup). - if (muxName && muxName !== primary) { - secondaries.push({ kind: 'tmux', label: muxName }); - } - // agent session id — show only when genuinely known: non-empty, not the codeman - // sessionId placeholder (claudeSessionId is seeded with the session id until a - // real agent message arrives), and not redundant with the primary label. - if (agentId && agentId !== sessionId && agentId !== primary) { - secondaries.push({ kind: 'agent', label: agentId }); - } - return { primary, secondaries }; -} - // COD-134 — Terminal WebSocket reconnect policy. // // Decide what to do after a terminal WebSocket closes, given the close `code` @@ -188,9 +156,6 @@ if (typeof window !== 'undefined') { window.CodemanTabOverflow = { shouldAutoWrapTabs, }; - window.CodemanMonitorLabels = { - resolveMonitorRowLabels, - }; window.CodemanWsReconnect = { plan: planWsReconnect, }; diff --git a/src/web/public/styles.css b/src/web/public/styles.css index bf0d3cdc..43aef54a 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -626,6 +626,15 @@ body { flex-shrink: 0; } +.connection-dot.connected { + background: var(--green); +} + +.connection-dot.fallback { + background: var(--yellow); + box-shadow: 0 0 6px var(--yellow); +} + .connection-dot.offline { background: var(--red); box-shadow: 0 0 6px var(--red); diff --git a/src/web/ws-connection-registry.ts b/src/web/ws-connection-registry.ts index d0c64cda..f73d229c 100644 --- a/src/web/ws-connection-registry.ts +++ b/src/web/ws-connection-registry.ts @@ -12,12 +12,15 @@ * 2. No clientId scoping: the limit counted raw sockets, so a reconnecting * client consumed a NEW slot instead of replacing its own. * - * This registry tracks the live socket(s) per session keyed by clientId (`cid`, - * parsed from the upgrade URL query). The reliable-input protocol - * (`session.shouldApplyInput(cid, seq)`) already assumes ONE logical client per - * `cid` per session, so a new upgrade for a `cid` that already holds a socket is - * a SUPERSEDE — the registry evicts the stale socket and reuses its slot, which - * makes a reconnect reclaim rather than double-count (fixes #1 and #2). + * This registry tracks the live socket(s) per session keyed by a per-TAB + * connection identity (`cid`, parsed from the upgrade URL query). The browser + * sends `clientId:tabNonce`, NOT the bare localStorage clientId — that one is + * shared by every tab/window of a profile, so keying on it would make two tabs + * on one session evict each other in a 4010 ping-pong. A new upgrade for a + * `cid` that already holds a socket is a SUPERSEDE — the registry evicts the + * stale socket and reuses its slot, which makes a reconnect reclaim rather + * than double-count (fixes #1 and #2). The cid is opaque here; input-frame + * dedup uses the bare clientId separately (`session.shouldApplyInput`). * * Backward-compat: an upgrade with NO `cid` (legacy clients, other tools) is * admitted anonymously — it counts toward the limit but never evicts another diff --git a/test/connection-indicator.test.ts b/test/connection-indicator.test.ts index 1c8a91ec..63477c2c 100644 --- a/test/connection-indicator.test.ts +++ b/test/connection-indicator.test.ts @@ -318,6 +318,21 @@ describe('_computeConnectionDescriptor — pure render per state (COD-136)', () }); }); +describe('connection-dot CSS — every emitted dot class has a styles.css rule', () => { + // The descriptor emits these dot variants; each needs a visible rule or the + // 8px dot renders as an invisible blob (the base .connection-dot rule has no + // background). 'connected' and 'fallback' were missing when this PR shipped. + const DOT_CLASSES = ['connected', 'fallback', 'offline', 'reconnecting', 'draining']; + const css = readFileSync(resolve(import.meta.dirname, '../src/web/public/styles.css'), 'utf8'); + + for (const cls of DOT_CLASSES) { + it(`.connection-dot.${cls} is styled`, () => { + const rule = new RegExp(`\\.connection-dot\\.${cls}\\s*\\{[^}]*background`, 'm'); + expect(css).toMatch(rule); + }); + } +}); + /** A DOM element fake that COUNTS each property write — used to detect the skip. */ function countingElement() { const writes = { display: 0, className: 0, textContent: 0, title: 0 }; diff --git a/test/ws-connection-registry.test.ts b/test/ws-connection-registry.test.ts index 35105878..db1a9322 100644 --- a/test/ws-connection-registry.test.ts +++ b/test/ws-connection-registry.test.ts @@ -100,6 +100,27 @@ describe('WsConnectionRegistry', () => { expect(reg.register('s1', 'alice', fresher).evictedSocket).toBe(fresh); }); + it('two tabs of the same browser (shared clientId, distinct tab nonce) coexist without eviction', () => { + // The client keys the upgrade by `clientId:tabNonce`, NOT the bare + // browser-wide clientId — otherwise two windows on one session would + // supersede each other in a perpetual 4010/5s reconnect ping-pong. + const reg = new WsConnectionRegistry(5); + const tabA = sock('tab-a'); + const tabB = sock('tab-b'); + + expect(reg.register('s1', 'c-browser:tab-A', tabA).evictedSocket).toBeUndefined(); + const resB = reg.register('s1', 'c-browser:tab-B', tabB); + expect(resB.admitted).toBe(true); + expect(resB.evictedSocket).toBeUndefined(); // tab A keeps its socket + expect(reg.liveCount('s1')).toBe(2); + + // A genuine same-tab reconnect still supersedes only its own socket. + const tabANew = sock('tab-a-new'); + const res = reg.register('s1', 'c-browser:tab-A', tabANew); + expect(res.evictedSocket).toBe(tabA); + expect(reg.liveCount('s1')).toBe(2); + }); + it('isolates counts per session', () => { const reg = new WsConnectionRegistry(2); reg.register('s1', 'a', sock('a')); diff --git a/test/ws-reconnect-plan.test.ts b/test/ws-reconnect-plan.test.ts index dbbfd37f..68ccd97f 100644 --- a/test/ws-reconnect-plan.test.ts +++ b/test/ws-reconnect-plan.test.ts @@ -6,8 +6,7 @@ * number of consecutive reconnects already attempted, it returns the action to * take (`reconnect` | `retry-fallback` | `give-up`) and a backoff delay. It is * exposed on `window.CodemanWsReconnect` and tested here in a plain node VM - * context (no jsdom — jsdom env setup is broken on some hosts), mirroring the - * `CodemanMonitorLabels` harness in test/monitor-row-labels.test.ts. + * context (no jsdom — jsdom env setup is broken on some hosts). */ import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; diff --git a/test/ws-state-lifecycle.test.ts b/test/ws-state-lifecycle.test.ts new file mode 100644 index 00000000..39923f3e --- /dev/null +++ b/test/ws-state-lifecycle.test.ts @@ -0,0 +1,293 @@ +/** + * @fileoverview Terminal WebSocket state-machine lifecycle tests + * (`CodemanApp._connectWs` / `ws.onopen` / `ws.onclose` / `_disconnectWs`). + * + * Unlike test/connection-indicator.test.ts (which pins the pure descriptor per + * pre-seeded `_wsState`), this suite drives the REAL transitions through a fake + * `WebSocket` class so the production assignments are covered: + * + * 1. `_connectWs()` → 'connecting', a real `onopen` → 'connected' (the chip + * renders "WS"), `_disconnectWs()` → 'disconnected'. Regression guard for + * the PR-review blocker where `_wsState` was only ever written in + * `onclose`, leaving the chip stuck on "WS…"/"HTTP" forever. + * 2. Exponential backoff really escalates across the onclose → timer → + * `_connectWs` cycle: `_disconnectWs()` (called first by `_connectWs`) + * must NOT zero `_wsReconnectAttempts`, or every retry replans at + * attempt 0 (a ~0ms tight reconnect loop during an outage). Only a + * successful `onopen` resets the counter. + * 3. The upgrade URL carries the per-TAB `cid` (`clientId:tabNonce`), not the + * browser-wide clientId — two tabs of one profile must register distinct + * registry keys so they coexist instead of 4010-evicting each other. + * + * Loaded via `vm` with a stubbed context (no jsdom — see input-send-order.test.ts). + */ +import { readFileSync } from 'node:fs'; +import { performance } from 'node:perf_hooks'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it, vi } from 'vitest'; + +type FakeTimer = { id: number; fn: () => void; delay: number; cleared: boolean }; + +class FakeWebSocket { + static OPEN = 1; + url: string; + readyState = 0; + closed = false; + onopen: (() => void) | null = null; + onmessage: ((e: unknown) => void) | null = null; + onclose: ((e: { code: number; reason: string }) => void) | null = null; + onerror: (() => void) | null = null; + + constructor(url: string) { + this.url = url; + FakeWebSocket.instances.push(this); + } + + send(): void {} + + close(): void { + this.closed = true; + this.readyState = 3; + } + + static instances: FakeWebSocket[] = []; +} + +function loadHarness() { + FakeWebSocket.instances = []; + const timers: FakeTimer[] = []; + let nextTimerId = 1; + const constants = readFileSync(resolve(import.meta.dirname, '../src/web/public/constants.js'), 'utf8'); + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + const context = vm.createContext({ + console, + performance, + setInterval: vi.fn(), + clearInterval: vi.fn(), + setTimeout: (fn: () => void, delay: number) => { + const id = nextTimerId++; + timers.push({ id, fn, delay, cleared: false }); + return id; + }, + clearTimeout: (id: number) => { + const t = timers.find((x) => x.id === id); + if (t) t.cleared = true; + }, + requestAnimationFrame: vi.fn(), + HTMLCanvasElement: class HTMLCanvasElement {}, + WebSocket: FakeWebSocket, + location: { protocol: 'https:', host: 'test.local' }, + fetch: (...args: Parameters) => global.fetch(...args), + document: { addEventListener: vi.fn() }, + localStorage: { + length: 0, + key: vi.fn(), + getItem: vi.fn(), + setItem: vi.fn(), + removeItem: vi.fn(), + }, + window: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + MobileDetection: {}, + }); + vm.runInContext(`${constants}\n${source}\nglobalThis.__CodemanApp = CodemanApp;`, context); + const CodemanApp = (context as { __CodemanApp: new () => unknown }).__CodemanApp; + return { CodemanApp, timers }; +} + +function fakeElement() { + return { style: { display: '' }, title: '', textContent: '', className: '' }; +} + +type LifecycleApp = { + _connectWs: (id: string) => void; + _disconnectWs: () => void; + _wsState: string; + _wsReady: boolean; + _wsReconnectAttempts: number | undefined; + _ws: FakeWebSocket | null; + activeSessionId: string | null; +}; + +function makeApp( + CodemanApp: new () => unknown, + overrides: Record = {} +): { app: LifecycleApp; els: Record> } { + const app = Object.create((CodemanApp as { prototype: object }).prototype) as LifecycleApp & Record; + const els: Record> = { + connectionIndicator: fakeElement(), + connectionDot: fakeElement(), + connectionText: fakeElement(), + }; + app.$ = (id: string) => els[id]; + app._clientId = 'c-browser'; + app._wsTabNonce = 'tab-1'; + app._ws = null; + app._wsSessionId = null; + app._wsReady = false; + app._wsState = 'disconnected'; + app._wsLastRecvAt = 0; + app._lastIndicatorDescriptor = null; + app._pendingDeliveries = new Map(); + app._connectionStatus = 'connected'; + app.activeSessionId = 's1'; + app.isOnline = true; + app.sendResize = vi.fn(); + app._onWsReady = vi.fn(); + Object.assign(app, overrides); + return { app: app as LifecycleApp, els }; +} + +/** Run the oldest pending (not-cleared, not-yet-fired) reconnect timer. */ +function fireNextTimer(timers: FakeTimer[]): FakeTimer { + const t = timers.find((x) => !x.cleared); + if (!t) throw new Error('no pending timer'); + t.cleared = true; // mark consumed so the next fire picks the following one + t.fn(); + return t; +} + +describe('WS state lifecycle — real _connectWs/onopen/onclose transitions', () => { + it("_connectWs sets 'connecting', a real onopen sets 'connected' and renders 'WS'", () => { + const { CodemanApp } = loadHarness(); + const { app, els } = makeApp(CodemanApp); + + app._connectWs('s1'); + expect(app._wsState).toBe('connecting'); + expect(els.connectionText.textContent).toBe('WS…'); + + const ws = FakeWebSocket.instances[0]; + ws.readyState = 1; + ws.onopen?.(); + + expect(app._wsState).toBe('connected'); + expect(app._wsReady).toBe(true); + expect(app._wsReconnectAttempts).toBe(0); + // The chip must show the healthy transport from the REAL open path — the + // 'connected' branch was dead code when only onclose wrote _wsState. + expect(els.connectionText.textContent).toBe('WS'); + expect(els.connectionDot.className).toBe('connection-dot connected'); + }); + + it("_disconnectWs resets the state machine to 'disconnected' and closes the socket", () => { + const { CodemanApp } = loadHarness(); + const { app } = makeApp(CodemanApp); + + app._connectWs('s1'); + const ws = FakeWebSocket.instances[0]; + ws.readyState = 1; + ws.onopen?.(); + expect(app._wsState).toBe('connected'); + + app._disconnectWs(); + expect(app._wsState).toBe('disconnected'); + expect(app._wsReady).toBe(false); + expect(app._ws).toBeNull(); + expect(ws.closed).toBe(true); + }); + + it("a retry-fallback close (4010) shows 'HTTP', and the successful retry returns the chip to 'WS'", () => { + const { CodemanApp, timers } = loadHarness(); + const { app, els } = makeApp(CodemanApp); + + app._connectWs('s1'); + const ws1 = FakeWebSocket.instances[0]; + ws1.readyState = 1; + ws1.onopen?.(); + expect(els.connectionText.textContent).toBe('WS'); + + ws1.onclose?.({ code: 4010, reason: 'Superseded by reconnect' }); + expect(app._wsState).toBe('fallback'); + expect(els.connectionText.textContent).toBe('HTTP'); + + // The bounded 5s retry succeeds → the chip must NOT stay stuck on "HTTP". + const timer = fireNextTimer(timers); + expect(timer.delay).toBe(5000); + const ws2 = FakeWebSocket.instances[1]; + ws2.readyState = 1; + ws2.onopen?.(); + expect(app._wsState).toBe('connected'); + expect(els.connectionText.textContent).toBe('WS'); + }); +}); + +describe('WS reconnect backoff — attempts survive the _connectWs → _disconnectWs call', () => { + it('escalates the transient-close delay ladder instead of replanning at attempt 0', () => { + const { CodemanApp, timers } = loadHarness(); + const { app } = makeApp(CodemanApp); + + app._connectWs('s1'); + // Attempt 0: transient close plans 0ms (+ <250ms jitter). + FakeWebSocket.instances[0].onclose?.({ code: 1006, reason: '' }); + expect(app._wsReconnectAttempts).toBe(1); + const t1 = fireNextTimer(timers); + expect(t1.delay).toBeLessThan(250); + + // Attempt 1: the retry's _connectWs ran _disconnectWs first — the counter + // must survive it, so this close plans 250ms (+ jitter), not 0ms again. + FakeWebSocket.instances[1].onclose?.({ code: 1006, reason: '' }); + expect(app._wsReconnectAttempts).toBe(2); + const t2 = fireNextTimer(timers); + expect(t2.delay).toBeGreaterThanOrEqual(250); + expect(t2.delay).toBeLessThan(500); + + // Attempt 2 → 500ms rung. + FakeWebSocket.instances[2].onclose?.({ code: 1006, reason: '' }); + expect(app._wsReconnectAttempts).toBe(3); + const t3 = fireNextTimer(timers); + expect(t3.delay).toBeGreaterThanOrEqual(500); + expect(t3.delay).toBeLessThan(750); + }); + + it('a successful onopen (not an intentional disconnect) is what resets the counter', () => { + const { CodemanApp, timers } = loadHarness(); + const { app } = makeApp(CodemanApp); + + app._connectWs('s1'); + FakeWebSocket.instances[0].onclose?.({ code: 1006, reason: '' }); + FakeWebSocket.instances[0].closed = true; + fireNextTimer(timers); + expect(app._wsReconnectAttempts).toBe(1); + + const ws2 = FakeWebSocket.instances[1]; + ws2.readyState = 1; + ws2.onopen?.(); + expect(app._wsReconnectAttempts).toBe(0); + expect(app._wsState).toBe('connected'); + }); +}); + +describe('WS upgrade cid — per-TAB identity (clientId:tabNonce)', () => { + it('sends the composite cid on the upgrade URL, keeping the bare clientId for input frames', () => { + const { CodemanApp } = loadHarness(); + const { app } = makeApp(CodemanApp); + + app._connectWs('s1'); + const url = new URL(FakeWebSocket.instances[0].url); + expect(url.searchParams.get('cid')).toBe('c-browser:tab-1'); + }); + + it('two tabs sharing the browser clientId register DIFFERENT registry keys', () => { + const { CodemanApp } = loadHarness(); + const { app: tabA } = makeApp(CodemanApp, { _wsTabNonce: 'tab-A' }); + const { app: tabB } = makeApp(CodemanApp, { _wsTabNonce: 'tab-B' }); + + tabA._connectWs('s1'); + tabB._connectWs('s1'); + + const cidA = new URL(FakeWebSocket.instances[0].url).searchParams.get('cid'); + const cidB = new URL(FakeWebSocket.instances[1].url).searchParams.get('cid'); + expect(cidA).toBe('c-browser:tab-A'); + expect(cidB).toBe('c-browser:tab-B'); + // Distinct keys → the server registry admits both instead of supersede-evicting. + expect(cidA).not.toBe(cidB); + }); + + it('omits the cid query entirely when no clientId is available', () => { + const { CodemanApp } = loadHarness(); + const { app } = makeApp(CodemanApp, { _clientId: '' }); + + app._connectWs('s1'); + expect(FakeWebSocket.instances[0].url).not.toContain('cid='); + }); +});