fix(review): WS state machine, per-tab supersede key, backoff, dot CSS (PR #149)

- _wsState now transitions through the full lifecycle: _connectWs() sets
  'connecting', ws.onopen (inside the this._ws === ws guard) sets 'connected',
  _disconnectWs() resets to 'disconnected' — the connection chip's "WS" state
  was previously unreachable (stuck on "WS…"/"HTTP" forever).
- WS registry supersede is now keyed per TAB: the upgrade URL sends
  cid = clientId + ':' + per-page nonce (reusing the constructor's page UUID),
  while input frames keep the bare browser clientId for seq dedup — two
  tabs/windows on one session coexist instead of 4010-evicting each other in a
  perpetual 5s ping-pong; a genuine same-tab reconnect still supersedes.
- Exponential backoff engages: _disconnectWs() no longer zeroes
  _wsReconnectAttempts (it's called at the top of _connectWs, so every retry
  replanned at attempt 0 → ~0ms tight reconnect loop during outages); onopen
  resets the counter on success.
- styles.css: add .connection-dot.connected (green) and .connection-dot.fallback
  (yellow) — both states rendered an invisible dot (no rule existed).
- Remove smuggled dead code: resolveMonitorRowLabels/CodemanMonitorLabels
  (COD-122, no consumer, referenced test doesn't exist) and the never-written
  _wsLastClose/_wsInputSendCount/_httpFallbackSendCount diagnostics.
- Tests: new test/ws-state-lifecycle.test.ts drives the REAL
  _connectWs/onopen/onclose/timer cycle (state transitions, escalating backoff
  delays, composite cid on the upgrade URL); registry two-tab coexistence test;
  static check that every emitted connection-dot class has a styles.css rule.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-07-12 18:42:36 +02:00
parent 584910f645
commit 6e417d69dc
8 changed files with 373 additions and 52 deletions
+25 -9
View File
@@ -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
-35
View File
@@ -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,
};
+9
View File
@@ -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);
+9 -6
View File
@@ -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
+15
View File
@@ -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 };
+21
View File
@@ -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'));
+1 -2
View File
@@ -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';
+293
View File
@@ -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<typeof fetch>) => 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<string, unknown> = {}
): { app: LifecycleApp; els: Record<string, ReturnType<typeof fakeElement>> } {
const app = Object.create((CodemanApp as { prototype: object }).prototype) as LifecycleApp & Record<string, unknown>;
const els: Record<string, ReturnType<typeof fakeElement>> = {
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=');
});
});