From 3397bfa9d19b91fca5a08e960cdb2ccfba169606 Mon Sep 17 00:00:00 2001 From: robertjamesprior <83608739+robertjamesprior@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:20:56 +0000 Subject: [PATCH] Report live view connect failures by stage, without retrying A live view that could not start reported nothing to the parent frame until a 15s watchdog fired, and a throw from peer construction was discarded outright, so an embedder could not tell a failed connect from a slow one. Bound the connect stages separately -- transport 15s, signaling 3s, media 2s -- and report the failure to the parent frame with a reason an embedder can branch on: KERNEL_CONNECTION_TIMEOUT when a bound expires, KERNEL_CONNECTION_FAILED when the connect fails outright. One event per failure, both carrying the ICE, signaling and socket state at the moment it happened. The client does not retry. A second attempt against the same peer costs the viewer time an embedder can spend on a new session, and the reason is what lets it make that call. --- .../chromium-headful/client/src/neko/base.ts | 111 ++++++- .../chromium-headful/client/src/neko/index.ts | 1 + .../client/tests/connect-bound.test.ts | 309 ++++++++++++++++++ 3 files changed, 411 insertions(+), 10 deletions(-) create mode 100644 images/chromium-headful/client/tests/connect-bound.test.ts diff --git a/images/chromium-headful/client/src/neko/base.ts b/images/chromium-headful/client/src/neko/base.ts index 5bacc96fe..70ec97121 100644 --- a/images/chromium-headful/client/src/neko/base.ts +++ b/images/chromium-headful/client/src/neko/base.ts @@ -11,6 +11,32 @@ import { SignalAnswerMessage, } from './messages' +// A connect walks transport -> signaling -> media. A single 15s clock over all +// three meant a socket that never opened burned the whole budget and reported +// only "timeout", so each stage fails on its own bound with its own reason. A +// bound expiring is terminal rather than retried: a second attempt against the +// same peer costs the viewer time an embedder can spend on a new session. +export type ConnectStage = 'transport' | 'signaling' | 'media' + +export const CONNECT_STAGE_TIMEOUT_MS: Record = { + // Network-bound (socket open + TLS), and the stage the live-view proxy's own + // wake path shows up in — it can spend ~12s waking a browser before the socket + // opens. Left at the watchdog it replaced rather than the measured p99, so a + // slow wake is not mistaken for a dead one. + transport: 15000, + // One server round trip on an already-open socket, plus the offer. Measured + // p99 under 200ms, including a cold session. + signaling: 3000, + // Local: ICE reaches `checking` as soon as the local description is set, since + // the remote candidates arrive in the offer. Measured 1-16ms direct, ~90ms + // relay-only, so this bound is ~20x the worst case rather than a guess. + media: 2000, +} + +// Sent to the parent frame so an embedder can pick a recovery without parsing +// prose. The three stages mean a bound expired; the rest never reached one. +export type ConnectFailure = ConnectStage | 'unsupported' | 'peer' | 'server' + export interface BaseEvents { info: (...message: any[]) => void warn: (...message: any[]) => void @@ -24,6 +50,12 @@ export abstract class BaseClient extends EventEmitter { protected _peer?: RTCPeerConnection protected _channel?: RTCDataChannel protected _timeout?: number + protected _stage?: ConnectStage + protected _everConnected = false + protected _gaveUp = false + // Tagged where the failure happens, so a pre-connect disconnect is reported + // with a reason an embedder can act on. Unset falls back to 'peer'. + protected _failure?: ConnectFailure protected _displayname?: string protected _state: RTCIceConnectionState = 'disconnected' protected _id = '' @@ -50,16 +82,22 @@ export abstract class BaseClient extends EventEmitter { } public connect(url: string, password: string, displayname: string) { + if (this._gaveUp) { + this.emit('debug', `not reconnecting, already gave up`) + return + } + if (this.socketOpen) { this.emit('warn', `attempting to create websocket while connection open`) return } if (!this.supported) { - this.onDisconnected(new Error('browser does not support webrtc (RTCPeerConnection missing)')) + this.giveUp('unsupported', new Error('browser does not support webrtc (RTCPeerConnection missing)')) return } + this._failure = undefined this._displayname = displayname this[EVENT.CONNECTING]() @@ -70,12 +108,15 @@ export abstract class BaseClient extends EventEmitter { this.emit('debug', `connecting to ${this._ws.url}`) this._ws.onmessage = this.onMessage.bind(this) this._ws.onerror = this.onError.bind(this) + this._ws.onopen = () => this.armStage('signaling') this._ws.onclose = (event) => { this.emit('debug', `websocket closed: code=${event.code}, reason=${event.reason}`) + this._failure = 'transport' this.onDisconnected(new Error('websocket closed')) } - this._timeout = window.setTimeout(this.onTimeout.bind(this), 15000) + this.armStage('transport') } catch (err: any) { + this._failure = 'transport' this.onDisconnected(err) } } @@ -85,6 +126,7 @@ export abstract class BaseClient extends EventEmitter { clearTimeout(this._timeout) this._timeout = undefined } + this._stage = undefined if (this._ws_heartbeat) { clearInterval(this._ws_heartbeat) @@ -344,7 +386,19 @@ export abstract class BaseClient extends EventEmitter { await this._peer.setRemoteDescription({ type: 'answer', sdp }) } + // onMessage is assigned straight to ws.onmessage, so a rejection here has + // nowhere to go: without this guard a throw from createPeer or + // setRemoteOffer is discarded and the client cannot tell "peer construction + // failed" from "still connecting". private async onMessage(e: MessageEvent) { + try { + await this.handleMessage(e) + } catch (err: unknown) { + this.onDisconnected(err instanceof Error ? err : new Error(String(err))) + } + } + + private async handleMessage(e: MessageEvent) { const { event, ...payload } = JSON.parse(e.data) as WebSocketMessages this.emit('debug', `received websocket event ${event} ${payload ? `with payload: ` : ''}`, payload) @@ -354,12 +408,14 @@ export abstract class BaseClient extends EventEmitter { this._id = id await this.createPeer(lite, ice) await this.setRemoteOffer(sdp) + this.armStage('media') return } if (event === EVENT.SIGNAL.OFFER) { const { sdp } = payload as SignalOfferPayload await this.setRemoteOffer(sdp) + this.armStage('media') return } @@ -433,28 +489,63 @@ export abstract class BaseClient extends EventEmitter { return } + this._everConnected = true + this.emit('debug', `connected`) this[EVENT.CONNECTED]() } + private armStage(stage: ConnectStage) { + if (this._timeout) { + clearTimeout(this._timeout) + } + + this._stage = stage + this._timeout = window.setTimeout(this.onTimeout.bind(this), CONNECT_STAGE_TIMEOUT_MS[stage]) + } + private onTimeout() { - this.emit('debug', `connection timeout`) + const stage = this._stage ?? 'transport' + this.emit('debug', `connection timeout at ${stage} stage`) + + if (this._timeout) { + clearTimeout(this._timeout) + this._timeout = undefined + } + + // The bound expiring is this connect's terminal event, so report it here and + // let onDisconnected take the ordinary disconnect path rather than reporting + // a second event for the same failure. + this.reportFailure('KERNEL_CONNECTION_TIMEOUT', stage) + this.onDisconnected(new Error(`${stage} timeout`)) + } + + private giveUp(failure: ConnectFailure, reason: Error) { + this.reportFailure('KERNEL_CONNECTION_FAILED', failure) + this.onDisconnected(reason) + } + + private reportFailure(type: 'KERNEL_CONNECTION_TIMEOUT' | 'KERNEL_CONNECTION_FAILED', reason: ConnectFailure) { + this._gaveUp = true this.postParentMessage({ - type: 'KERNEL_CONNECTION_TIMEOUT', - reason: 'connection timeout', + type, + reason, iceConnectionState: this._peer?.iceConnectionState ?? this._state, connectionState: this._peer?.connectionState, signalingState: this._peer?.signalingState, socketOpen: this.socketOpen, }) - if (this._timeout) { - clearTimeout(this._timeout) - this._timeout = undefined - } - this.onDisconnected(new Error('connection timeout')) } protected onDisconnected(reason?: Error) { + // A disconnect before any peer was established is a failed connect, not a + // dropped session. disconnect() clears the bound below, so without this the + // parent frame would hear nothing at all. + if (!this._gaveUp && !this._everConnected) { + this.giveUp(this._failure ?? 'peer', reason ?? new Error('connection failed')) + return + } + this.disconnect() this.emit('debug', `disconnected:`, reason) this[EVENT.DISCONNECTED](reason) diff --git a/images/chromium-headful/client/src/neko/index.ts b/images/chromium-headful/client/src/neko/index.ts index 3571bb88e..9c7463483 100644 --- a/images/chromium-headful/client/src/neko/index.ts +++ b/images/chromium-headful/client/src/neko/index.ts @@ -163,6 +163,7 @@ export class NekoClient extends BaseClient implements EventEmitter { message = this.$vue.$t('connection.kicked') as string } + this._failure = 'server' this.onDisconnected(new Error(message)) this.$vue.$swal({ diff --git a/images/chromium-headful/client/tests/connect-bound.test.ts b/images/chromium-headful/client/tests/connect-bound.test.ts new file mode 100644 index 000000000..4d5ae9b1b --- /dev/null +++ b/images/chromium-headful/client/tests/connect-bound.test.ts @@ -0,0 +1,309 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { BaseClient, CONNECT_STAGE_TIMEOUT_MS } from '../src/neko/base' + +const posted: Record[] = [] +const timers: { id: number; delay: number; run: () => void }[] = [] +const realClearTimeout = globalThis.clearTimeout +let nextTimerId = 1 + +// The client mixes `window.setTimeout` with a bare `clearTimeout`, so the global +// has to be stubbed too for a cleared stage bound to actually be cancelled. +function clearTimer(id: number) { + const index = timers.findIndex((timer) => timer.id === id) + if (index !== -1) timers.splice(index, 1) +} + +function runTimers(delay: number) { + for (const timer of [...timers]) { + if (timer.delay !== delay) continue + clearTimer(timer.id) + timer.run() + } +} + +let lastPeer: FakePeer | undefined + +class FakePeer { + iceConnectionState = 'new' + connectionState = 'new' + signalingState = 'stable' + onconnectionstatechange: () => void = () => {} + onsignalingstatechange: () => void = () => {} + oniceconnectionstatechange: () => void = () => {} + onicecandidate: (event: RTCPeerConnectionIceEvent) => void = () => {} + onnegotiationneeded: () => void = () => {} + ontrack: (event: RTCTrackEvent) => void = () => {} + async setRemoteDescription() {} + async setLocalDescription() {} + async createAnswer() { + return { sdp: 'v=0' } + } + async createOffer() { + return { sdp: 'v=0' } + } + async addIceCandidate() {} + createDataChannel() { + return { onerror: () => {}, onmessage: () => {}, onclose: () => {}, close: () => {} } + } + close() {} +} + +class FakeSocket { + static OPEN = 1 + readyState = 0 + onopen: () => void = () => {} + onmessage: (e: MessageEvent) => void = () => {} + onerror: (e: Event) => void = () => {} + onclose: (e: CloseEvent) => void = () => {} + constructor(public url: string) {} + send() {} + close() { + this.readyState = 3 + } +} + +class TestClient extends BaseClient { + reasons: (Error | undefined)[] = [] + protected RECONNECTING() {} + protected CONNECTING() {} + protected CONNECTED() {} + protected DISCONNECTED(reason?: Error) { + this.reasons.push(reason) + } + protected TRACK() {} + protected DATA() {} + + provide() { + return this['onMessage']({ + data: JSON.stringify({ event: 'signal/provide', id: 'x', lite: false, ice: [], sdp: 'v=0' }), + } as MessageEvent) + } + + openSocket() { + this['_ws']!.readyState = FakeSocket.OPEN + this['_ws']!.onopen() + } +} + +function setPeerConstructor(impl: () => unknown) { + const ctor = function () { + lastPeer = (impl() ?? {}) as FakePeer + return lastPeer + } as unknown as typeof RTCPeerConnection + ctor.prototype = { addTransceiver() {} } + Object.defineProperty(globalThis, 'RTCPeerConnection', { value: ctor, configurable: true }) +} + +beforeEach(() => { + posted.length = 0 + timers.length = 0 + lastPeer = undefined + nextTimerId = 1 + const parent = { postMessage: (m: Record) => posted.push(m) } + Object.defineProperty(globalThis, 'window', { + value: { + parent, + setTimeout: (run: () => void, delay: number) => { + const id = nextTimerId++ + timers.push({ id, delay, run }) + return id + }, + clearTimeout: clearTimer, + }, + configurable: true, + }) + globalThis.clearTimeout = clearTimer + Object.defineProperty(globalThis, 'document', { value: { referrer: '' }, configurable: true }) + Object.defineProperty(globalThis, 'WebSocket', { value: FakeSocket, configurable: true }) + setPeerConstructor(() => new FakePeer()) +}) + +afterEach(() => { + globalThis.clearTimeout = realClearTimeout + for (const key of ['window', 'document', 'WebSocket', 'RTCPeerConnection']) { + Reflect.deleteProperty(globalThis, key) + } +}) + +describe('live view connect failures', () => { + test('a throw from peer construction is reported with reason peer', async () => { + setPeerConstructor(() => { + throw new Error('RTCPeerConnection blocked') + }) + + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + expect(posted).toEqual([ + { + type: 'KERNEL_CONNECTION_FAILED', + reason: 'peer', + iceConnectionState: 'disconnected', + connectionState: undefined, + signalingState: undefined, + socketOpen: true, + }, + ]) + expect(client.reasons.map((r) => r?.message)).toEqual(['RTCPeerConnection blocked']) + }) + + test('a socket that closes before any peer is established reports reason transport', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client['_ws']!.readyState = FakeSocket.OPEN + + client['_ws']!.onclose({ code: 1005, reason: '' } as CloseEvent) + + expect(posted).toHaveLength(1) + expect(posted[0].type).toBe('KERNEL_CONNECTION_FAILED') + expect(posted[0].reason).toBe('transport') + }) + + test('an unsupported browser fails once and opens no socket', () => { + Reflect.deleteProperty(globalThis, 'RTCPeerConnection') + const client = new TestClient() + + client.connect('ws://host/ws', 'pw', 'kernel') + client.connect('ws://host/ws', 'pw', 'kernel') + + expect(posted).toHaveLength(1) + expect(posted[0].reason).toBe('unsupported') + expect(client['_ws']).toBeUndefined() + }) + + test('a transport stage timeout reports one event, tagged with the stage', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + + expect(client['_stage']).toBe('transport') + + runTimers(CONNECT_STAGE_TIMEOUT_MS.transport) + + expect(posted.map((m) => m.type)).toEqual(['KERNEL_CONNECTION_TIMEOUT']) + expect(posted[0].reason).toBe('transport') + }) + + test('the signaling stage starts when the socket opens and keeps its own reason', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + + expect(client['_stage']).toBe('signaling') + + runTimers(CONNECT_STAGE_TIMEOUT_MS.signaling) + + expect(posted.map((m) => m.type)).toEqual(['KERNEL_CONNECTION_TIMEOUT']) + expect(posted[0].reason).toBe('signaling') + }) + + test('after signal/provide the media stage starts and reports its own reason', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + expect(client['_stage']).toBe('media') + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted).toHaveLength(1) + expect(posted[0].type).toBe('KERNEL_CONNECTION_TIMEOUT') + expect(posted[0].reason).toBe('media') + }) + + test('ICE reaching checking clears the stage bound', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + lastPeer!.iceConnectionState = 'checking' + client['_peer']!.oniceconnectionstatechange() + + expect(client['_timeout']).toBeUndefined() + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted).toEqual([]) + }) + + test('a stage timeout is not followed by a second event for the same connect', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + runTimers(CONNECT_STAGE_TIMEOUT_MS.transport) + + client['onDisconnected'](new Error('late close')) + + expect(posted).toHaveLength(1) + }) + + test('the payload carries the connection state at the moment of failure', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted[0]).toEqual({ + type: 'KERNEL_CONNECTION_TIMEOUT', + reason: 'media', + iceConnectionState: 'new', + connectionState: 'new', + signalingState: 'stable', + socketOpen: true, + }) + expect('attempts' in posted[0]).toBe(false) + }) + + test('an untagged pre-connect disconnect falls back to reason peer', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + + client['onDisconnected'](new Error('peer failed')) + + expect(posted).toHaveLength(1) + expect(posted[0].reason).toBe('peer') + }) + + test('a server-initiated disconnect before connecting keeps its own reason', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + + client['_failure'] = 'server' + client['onDisconnected'](new Error('kicked')) + + expect(posted).toHaveLength(1) + expect(posted[0].reason).toBe('server') + }) + + test('a disconnect after connecting is reported as a disconnect, not a connect failure', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client['_ws']!.readyState = FakeSocket.OPEN + client['_peer'] = {} as RTCPeerConnection + client['_state'] = 'connected' + client['onConnected']() + + client['onDisconnected'](new Error('network blip')) + + expect(posted).toEqual([]) + expect(client.reasons.map((r) => r?.message)).toEqual(['network blip']) + }) + + test('stops opening sockets once it has given up', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client['_ws']!.readyState = FakeSocket.OPEN + client['_ws']!.onclose({ code: 1005, reason: '' } as CloseEvent) + + const socket = client['_ws'] + client.connect('ws://host/ws', 'pw', 'kernel') + + expect(client['_ws']).toBeUndefined() + expect(socket).toBeUndefined() + expect(posted).toHaveLength(1) + }) +})