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 1/3] 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) + }) +}) From 9c222e9fcd769a578cf488aa9add71e279de6850 Mon Sep 17 00:00:00 2001 From: robertjamesprior <83608739+robertjamesprior@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:39:37 +0000 Subject: [PATCH 2/3] Stop the media bound re-arming after the peer connects A later signal/offer armed the two-second media bound even with ICE already connected, starting a timer no state change could clear that then reported a timeout and tore down a live session. Arm it only while the first connect is still in flight, and reset the per-attempt flag in connect() so a prior session's success does not suppress the next connect's bound. disconnect() also left the socket's onopen handler attached, so a socket that opened as the close ran could re-arm the signaling bound and post a second failure for a connect that had already given up. --- .../chromium-headful/client/src/neko/base.ts | 20 +++++- .../client/tests/connect-bound.test.ts | 62 +++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/images/chromium-headful/client/src/neko/base.ts b/images/chromium-headful/client/src/neko/base.ts index 70ec97121..fd426dc90 100644 --- a/images/chromium-headful/client/src/neko/base.ts +++ b/images/chromium-headful/client/src/neko/base.ts @@ -98,6 +98,9 @@ export abstract class BaseClient extends EventEmitter { } this._failure = undefined + // Per connect attempt: a prior session's success must not suppress this + // attempt's bound or make onDisconnected treat it as a dropped session. + this._everConnected = false this._displayname = displayname this[EVENT.CONNECTING]() @@ -135,6 +138,7 @@ export abstract class BaseClient extends EventEmitter { if (this._ws) { // reset all events + this._ws.onopen = () => {} this._ws.onmessage = () => {} this._ws.onerror = () => {} this._ws.onclose = () => {} @@ -408,14 +412,14 @@ export abstract class BaseClient extends EventEmitter { this._id = id await this.createPeer(lite, ice) await this.setRemoteOffer(sdp) - this.armStage('media') + this.armMediaStage() return } if (event === EVENT.SIGNAL.OFFER) { const { sdp } = payload as SignalOfferPayload await this.setRemoteOffer(sdp) - this.armStage('media') + this.armMediaStage() return } @@ -504,6 +508,18 @@ export abstract class BaseClient extends EventEmitter { this._timeout = window.setTimeout(this.onTimeout.bind(this), CONNECT_STAGE_TIMEOUT_MS[stage]) } + // The media bound exists to catch a connect whose peer never reaches ICE + // `checking`, and it is cleared when ICE does. Once the peer is connecting or + // already connected there is no first connect left to bound, and a later + // signal/offer would otherwise start a timer no future state change can clear, + // tearing down a live session. + private armMediaStage() { + if (this._gaveUp || this._everConnected || this.peerConnected) { + return + } + this.armStage('media') + } + private onTimeout() { const stage = this._stage ?? 'transport' this.emit('debug', `connection timeout at ${stage} stage`) diff --git a/images/chromium-headful/client/tests/connect-bound.test.ts b/images/chromium-headful/client/tests/connect-bound.test.ts index 4d5ae9b1b..683447336 100644 --- a/images/chromium-headful/client/tests/connect-bound.test.ts +++ b/images/chromium-headful/client/tests/connect-bound.test.ts @@ -79,6 +79,17 @@ class TestClient extends BaseClient { } as MessageEvent) } + offer() { + return this['onMessage']({ + data: JSON.stringify({ event: 'signal/offer', sdp: 'v=0' }), + } as MessageEvent) + } + + connectPeer() { + this['_peer']!.iceConnectionState = 'connected' + this['_peer']!.oniceconnectionstatechange() + } + openSocket() { this['_ws']!.readyState = FakeSocket.OPEN this['_ws']!.onopen() @@ -306,4 +317,55 @@ describe('live view connect failures', () => { expect(socket).toBeUndefined() expect(posted).toHaveLength(1) }) + + test('a later signal/offer after connecting does not re-arm the media bound', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + lastPeer!.iceConnectionState = 'checking' + client['_peer']!.oniceconnectionstatechange() + client.connectPeer() + + await client.offer() + + expect(client['_timeout']).toBeUndefined() + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted).toEqual([]) + }) + + test('a renegotiation offer while connecting is still bounded', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + await client.offer() + + expect(client['_stage']).toBe('media') + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted.map((m) => m.reason)).toEqual(['media']) + }) + + test('a socket that opens after a transport timeout opens no new bound', () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + const socket = client['_ws']! + + runTimers(CONNECT_STAGE_TIMEOUT_MS.transport) + + expect(posted.map((m) => m.type)).toEqual(['KERNEL_CONNECTION_TIMEOUT']) + + socket.readyState = FakeSocket.OPEN + socket.onopen() + + runTimers(CONNECT_STAGE_TIMEOUT_MS.signaling) + + expect(posted).toHaveLength(1) + }) }) From 3c72c246a7ca5f7a981466d94deb0d0876a6413a Mon Sep 17 00:00:00 2001 From: robertjamesprior <83608739+robertjamesprior@users.noreply.github.com> Date: Wed, 30 Sep 2026 17:44:09 +0000 Subject: [PATCH 3/3] Keep the media bound armed until ICE connects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `oniceconnectionstatechange` cleared the connect bound on `checking`, but `onConnected` — and `KERNEL_CONNECTED` — wait for `connected`. A peer that reached `checking` and then stalled had no timer watching and reported nothing to the parent frame, which is the silent black frame this branch exists to remove. Leaving the bound armed costs nothing on the healthy path: ICE reaches `connected` tens of milliseconds after `checking`, inside the same 2s budget, and `onConnected` clears it either way. --- .../chromium-headful/client/src/neko/base.ts | 26 +++++++++---------- .../client/tests/connect-bound.test.ts | 18 ++++++++++++- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/images/chromium-headful/client/src/neko/base.ts b/images/chromium-headful/client/src/neko/base.ts index fd426dc90..cdb95e78c 100644 --- a/images/chromium-headful/client/src/neko/base.ts +++ b/images/chromium-headful/client/src/neko/base.ts @@ -28,8 +28,10 @@ export const CONNECT_STAGE_TIMEOUT_MS: Record = { // 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. + // the remote candidates arrive in the offer — measured 1-16ms direct, ~90ms + // relay-only. The bound covers through `connected` rather than stopping at + // `checking`, so it also catches a peer that reaches `checking` and then + // stalls; that stretch is tens of milliseconds, inside the same budget. media: 2000, } @@ -284,12 +286,10 @@ export abstract class BaseClient extends EventEmitter { this.emit('debug', `peer ice connection state changed: ${this._peer!.iceConnectionState}`) switch (this._state) { - case 'checking': - if (this._timeout) { - clearTimeout(this._timeout) - this._timeout = undefined - } - break + // `checking` deliberately leaves the media bound armed. It only means the + // peer started connecting, and onConnected — and KERNEL_CONNECTED — wait + // for `connected`, so clearing here left a connect that stalled in + // between with no timer watching and nothing reported. case 'connected': this.onConnected() break @@ -508,11 +508,11 @@ export abstract class BaseClient extends EventEmitter { this._timeout = window.setTimeout(this.onTimeout.bind(this), CONNECT_STAGE_TIMEOUT_MS[stage]) } - // The media bound exists to catch a connect whose peer never reaches ICE - // `checking`, and it is cleared when ICE does. Once the peer is connecting or - // already connected there is no first connect left to bound, and a later - // signal/offer would otherwise start a timer no future state change can clear, - // tearing down a live session. + // The media bound covers the connect up to ICE `connected`, and onConnected + // clears it. A later signal/offer must not start a fresh bound: once the peer + // is connected there is no first connect left to bound, and a timer armed then + // is one no future state change can clear, which would tear down a live + // session. private armMediaStage() { if (this._gaveUp || this._everConnected || this.peerConnected) { return diff --git a/images/chromium-headful/client/tests/connect-bound.test.ts b/images/chromium-headful/client/tests/connect-bound.test.ts index 683447336..fd26180bd 100644 --- a/images/chromium-headful/client/tests/connect-bound.test.ts +++ b/images/chromium-headful/client/tests/connect-bound.test.ts @@ -224,7 +224,7 @@ describe('live view connect failures', () => { expect(posted[0].reason).toBe('media') }) - test('ICE reaching checking clears the stage bound', async () => { + test('ICE reaching checking leaves the media bound armed and reports it if it stalls', async () => { const client = new TestClient() client.connect('ws://host/ws', 'pw', 'kernel') client.openSocket() @@ -233,6 +233,22 @@ describe('live view connect failures', () => { lastPeer!.iceConnectionState = 'checking' client['_peer']!.oniceconnectionstatechange() + expect(client['_timeout']).toBeDefined() + + runTimers(CONNECT_STAGE_TIMEOUT_MS.media) + + expect(posted.map((m) => m.type)).toEqual(['KERNEL_CONNECTION_TIMEOUT']) + expect(posted[0].reason).toBe('media') + }) + + test('ICE reaching connected clears the media bound', async () => { + const client = new TestClient() + client.connect('ws://host/ws', 'pw', 'kernel') + client.openSocket() + await client.provide() + + client.connectPeer() + expect(client['_timeout']).toBeUndefined() runTimers(CONNECT_STAGE_TIMEOUT_MS.media)