From 0052e6fdec00e54cef5d418eb3d441aa59b0e741 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Thu, 13 Aug 2026 04:29:33 -0700 Subject: [PATCH] [eric] ws: a session id we never issue now names its own producer instead of silently 404ing (ENG-205) --- frontend/src/shared/ws/WebSocketManager.ts | 3 + frontend/src/shared/ws/sessionIdShape.test.ts | 65 +++++++++++++++++++ frontend/src/shared/ws/sessionIdShape.ts | 43 ++++++++++++ 3 files changed, 111 insertions(+) create mode 100644 frontend/src/shared/ws/sessionIdShape.test.ts create mode 100644 frontend/src/shared/ws/sessionIdShape.ts diff --git a/frontend/src/shared/ws/WebSocketManager.ts b/frontend/src/shared/ws/WebSocketManager.ts index fe711069..b9da563d 100644 --- a/frontend/src/shared/ws/WebSocketManager.ts +++ b/frontend/src/shared/ws/WebSocketManager.ts @@ -1,4 +1,5 @@ import { store } from '../state/store'; +import { warnIfNotCanonicalSessionId } from './sessionIdShape'; import { unstable_batchedUpdates } from 'react-dom'; import { updateSession, @@ -1020,6 +1021,7 @@ export function seedSessionSeq(sessionId: string, seq: number): void { } export function createSessionWs(sessionId: string): WebSocketManager { + warnIfNotCanonicalSessionId(sessionId, 'createSessionWs'); return new WebSocketManager(`${WS_BASE}/ws/agents/${sessionId}`, { sessionId }); } @@ -1032,6 +1034,7 @@ export function acquireSessionWs(sessionId: string): WebSocketManager { _backgroundedSessionWs = null; return ws; } + warnIfNotCanonicalSessionId(sessionId, 'acquireSessionWs'); return new WebSocketManager(`${WS_BASE}/ws/agents/${sessionId}`, { sessionId }); } diff --git a/frontend/src/shared/ws/sessionIdShape.test.ts b/frontend/src/shared/ws/sessionIdShape.test.ts new file mode 100644 index 00000000..f975fe67 --- /dev/null +++ b/frontend/src/shared/ws/sessionIdShape.test.ts @@ -0,0 +1,65 @@ +// Run: npm test +// +// ENG-205. A user saw `/api/agents/sessions/ae8813e9d5d20fb7.1 -> 404` plus a WS close. Measured +// across 248 real sessions: zero ids of that shape, and neither side constructs one. The point of +// this check is that the NEXT occurrence names its own producer. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { + isCanonicalSessionId, + warnIfNotCanonicalSessionId, + resetSessionIdWarnings, +} from './sessionIdShape.ts'; + +const REAL = 'ae8813e9d5d20fb7ae8813e9d5d20fb7'; // 32 hex, the shape we issue +const REPORTED = 'ae8813e9d5d20fb7.1'; // exactly what the user's console showed + +test('a real session id passes', () => { + assert.equal(isCanonicalSessionId(REAL), true); +}); + +test('the id from the report is rejected', () => { + assert.equal(isCanonicalSessionId(REPORTED), false, 'the reported id must be recognised as foreign'); +}); + +test('the near misses are all caught, since the point is the SHAPE', () => { + for (const bad of [ + 'ae8813e9d5d20fb7', // 16 hex, half length + 'ae8813e9d5d20fb7ae8813e9d5d20fb7.1', // full length plus a suffix + 'AE8813E9D5D20FB7AE8813E9D5D20FB7', // uppercase + 'ae8813e9d5d20fb7ae8813e9d5d20fbg', // non-hex char + '', + ]) { + assert.equal(isCanonicalSessionId(bad), false, `${JSON.stringify(bad)} should not pass`); + } +}); + +test('warning fires once per id, so a reconnect loop cannot flood the console', () => { + resetSessionIdWarnings(); + const seen: unknown[][] = []; + const orig = console.warn; + console.warn = (...a: unknown[]) => { seen.push(a); }; + try { + warnIfNotCanonicalSessionId(REPORTED, 'createSessionWs'); + warnIfNotCanonicalSessionId(REPORTED, 'createSessionWs'); + warnIfNotCanonicalSessionId(REPORTED, 'createSessionWs'); + } finally { + console.warn = orig; + } + assert.equal(seen.length, 1, `warned ${seen.length} times for one id`); + assert.match(String(seen[0][0]), /ENG-205/); + assert.match(String(seen[0][0]), /never issue/); +}); + +test('a good id never warns', () => { + resetSessionIdWarnings(); + let warned = 0; + const orig = console.warn; + console.warn = () => { warned += 1; }; + try { + assert.equal(warnIfNotCanonicalSessionId(REAL, 'createSessionWs'), true); + } finally { + console.warn = orig; + } + assert.equal(warned, 0, 'a legitimate id must be silent, or the warning becomes noise'); +}); diff --git a/frontend/src/shared/ws/sessionIdShape.ts b/frontend/src/shared/ws/sessionIdShape.ts new file mode 100644 index 00000000..33af1b32 --- /dev/null +++ b/frontend/src/shared/ws/sessionIdShape.ts @@ -0,0 +1,43 @@ +// Is this actually one of our session ids? (ENG-205) +// +// A user's console showed `GET /api/agents/sessions/ae8813e9d5d20fb7.1 -> 404` plus a matching +// "WebSocket closed before the connection was established". That id is 16 hex chars with a dotted +// suffix; ours are 32 hex chars with no suffix. Measured across 248 real sessions on a dev machine: +// zero filenames of that shape and zero `sdk_session_id` values containing a dot. Neither the +// frontend nor the backend constructs one, so the value arrives from somewhere as the sessionId and +// nothing on the way to the socket would notice. +// +// The bug is not the 404, it is that nobody can say who produced the id. So the shape check exists +// to make the next occurrence name its own producer instead of costing another round of guessing. + +/** Our session ids: 32 lowercase hex characters, nothing else. */ +const P_CANONICAL = /^[0-9a-f]{32}$/; + +export function isCanonicalSessionId(id: string): boolean { + return P_CANONICAL.test(id); +} + +/** + * Warn once per offending id, with a stack, when something asks us to open a socket for an id we + * could never have issued. Deliberately does NOT refuse: 248 sessions on one machine is not enough + * evidence to start dropping connections, and a diagnostic that changes behaviour is a second bug. + */ +const p_warned = new Set(); + +export function warnIfNotCanonicalSessionId(id: string, where: string): boolean { + if (isCanonicalSessionId(id)) return true; + if (p_warned.has(id)) return false; + p_warned.add(id); + // eslint-disable-next-line no-console + console.warn( + `[ENG-205] ${where} received a session id we never issue: ${JSON.stringify(id)} ` + + `(expected 32 hex chars). The 404 and the WS close that follow are consequences, not the bug. ` + + `Stack names the producer:`, + new Error('session-id origin').stack, + ); + return false; +} + +export function resetSessionIdWarnings(): void { + p_warned.clear(); +}