Report live view connect failures by stage, without retrying - #417
robertjamesprior wants to merge 2 commits into
Conversation
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.
The client reports a failed connect to the parent frame as a single terminal event, and both terminal events now carry a machine-readable reason instead of prose. - add KERNEL_CONNECTION_FAILED to the parent-frame events table - document the shared reason set -- transport, signaling, media, peer, unsupported, server -- so an embedder can branch on it without matching message text - note that a failed connect posts one of the two terminal events, never both, and that the client does not retry a connect itself - treat KERNEL_CONNECTION_TIMEOUT as terminal alongside it, and record the older reason string on images that predate the fix Version-gated on the browser image carrying kernel/kernel-images#417.
Sayan-
left a comment
There was a problem hiding this comment.
- p1: The client and dashboard use different failure events. A peer-construction error sends only
KERNEL_CONNECTION_FAILED, but the dashboard listens only forKERNEL_CONNECTION_TIMEOUT. Its failure panel and “Try again” button never appear; the viewer stays on an empty connect overlay. - p2: The field diagnosis rests on 15-second WebSocket closures, which show the watchdog firing but not why. No browser error was captured to confirm that peer construction threw in the affected sessions. The injected failure verifies the code defect, not the claimed field trigger.
robertjamesprior
left a comment
There was a problem hiding this comment.
p1 - accepted - this is a regression, takes priority. checked against browser-session-client.tsx, fixed in kernel/kernel#4421, which accepts both events and is compatible with either image (goes first)
p2 - accepted - no capture from those sessions, softened the claim in the issue and the PR body, and added a "what this does not establish" section listing the three causes the signature can't separate. the fix doesn't depend on resolving it
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3397bfa. Configure here.
Sayan-
left a comment
There was a problem hiding this comment.
- p2: A later
signal/offerrearms the two-second media timeout even after the peer has connected. If ICE stays connected without another state-change event, the timer reports a timeout and disconnects. The server can send an offer on renegotiation, but I could not establish that routine operations trigger one. I reproduced the client path with a simulated peer, not a live browser.
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.
|
p2 — fixed, and it was a real one. The media bound is now armed only while the first connect is still in flight: a later While I was in there I took the two Bugbot findings on the same path, since they're the same class of stale-bound problem:
Three tests added in The p1 (dashboard recognising both events) is kernel/kernel#4421, unchanged and still the one that goes first. |
|
Stacked a fix for the one stretch the bounds missed: #427.
Not included: a stall that begins after playback. That needs a frame-progress signal, and the peer path has no access to the media element, so it lands in the video layer as a separate change. |

Supersedes #403 and #415 — that stack collapsed into one change, since the retry the base PR added is gone. Calibration, reproduction and the hotpatch A/B are in the Linear doc.
Summary
The live view failed silently.
onMessageis assigned straight tows.onmessage, so a throw fromcreatePeerorsetRemoteOfferwas discarded: noonDisconnected, no parent-frame message, socket still open and healthy. The client could not tell "peer construction failed" from "still connecting."A single 15s watchdog also covered all three connect stages, so a socket that never opened burned the whole budget and reported only
"connection timeout", and a stuck media path was indistinguishable from a stuck socket.Route the throw.
onMessagewraps the handler, so a rejection reachesonDisconnectedand is reported instead of dropped.Bound each stage. A connect walks transport (socket open) → signaling (
signal/provide+ offer) → media (ICE starts). Each arms its own bound and reports its own reason.transportconnect(), before the socket openssignalingws.onopenmediasetRemoteOffer, once a peer existsReport one terminal event with a machine-readable reason. A bound expiring posts
KERNEL_CONNECTION_TIMEOUT; a connect that fails outright postsKERNEL_CONNECTION_FAILED. Both carryreasonand the ICE, signaling and socket state at the moment it happened.reasontransportsignalingmediacheckingpeersetRemoteOfferthrewunsupportedRTCPeerConnectionis missing from the browserserverNo retries. A second attempt against the same peer costs the viewer time the embedder can spend on a new session, and it delays the reason. The client reports the first failure; the embedder decides.
reasonreplaces the previous prose ("connection timeout", raw error messages) on both events, andattemptsis gone with the retry — both are payload changes.Consequence for embedders
A failed connect now ends within one stage bound instead of a silent 15s wait, with the reason on the parent frame. The neko overlay does not render a terminal state when a connect never succeeded, so an embedder that shows nothing still shows nothing; the parent message is the signal, and it is what lets an embedder choose between remounting and starting a new session.
One result that changes other work: in an injected
media-stall(offer stripped, ICE never leavesnew),KERNEL_PLAYINGstill fired while the peer had never connected — the<video>element emitsplayingwithout frames flowing. It did not fire in the reproduction (aRTCPeerConnectionconstruction throw). A parent-side gate on playback needs that caveat.Testing
35 pass, 0 failinimages/chromium-headful/client(bun test tests), including 13 new cases intests/connect-bound.test.tscovering stage entry, each stage's terminal event, the single-report guarantee, the payload state, and the post-connect disconnect path.tsc --noEmitclean.The hotpatch A/B numbers in the doc were taken against the stacked build that still retried. Removing the retry changes only the timeout rows, which now post a single event rather than
TIMEOUTfollowed byFAILED. They were not re-run on this revision.What this does not establish
The affected sessions are a ws close at ~14.9s, which proves the connect watchdog fired and therefore that ICE never reached
checking. It does not say why: no peer constructed, a peer that never leftnew, and signaling that never completed all produce that shape, and no browser-side capture exists from those sessions. The reproduction proves a construction throw produces the shape, not that it is what happened in the field.The change does not depend on resolving that.
reasonis what separates the three at the next occurrence, which is the point of the field.Dependency
The dashboard's live view panel recognises only
KERNEL_CONNECTION_TIMEOUT:so the failures that now report as
KERNEL_CONNECTION_FAILEDstop showing its failure panel, itsTry againbutton and its GPU failure inspection. kernel/kernel#4421 accepts both and must land first — accepting both is compatible with either image, so the dashboard goes first and nothing has to be released in lockstep.Not in this PR
Terminal UI with one-click resume, one shared recovery heuristic with the dashboard viewer, a frames-based gate, and awaiting
addIceCandidate.Note
Medium Risk
Changes WebRTC connect lifecycle, parent
postMessagepayload contract, and failure semantics for all live-view embedders; misaligned dashboard handling could hide failure UI until kernel#4421 lands.Overview
Replaces the live-view client’s single 15s connect watchdog with per-stage timeouts (
transport15s,signaling3s,media2s) and reports one terminal parent-frame event per failed connect—no in-client retry after give-up.WebSocket handler errors are no longer swallowed:
onMessagewraps the async handler so peer construction /setRemoteOfferfailures reachonDisconnectedand embedders getKERNEL_CONNECTION_FAILEDwith a machine-readablereason(transport,signaling,media,peer,unsupported,server) plus ICE/signaling/socket snapshot. Stage expiry postsKERNEL_CONNECTION_TIMEOUTwith the samereasonshape (replacing prose"connection timeout"and droppingattempts). Pre-connect failures always notify the parent; post-connect drops use the normal disconnect path. Server kicks set_failure = 'server'inindex.ts.Adds
connect-bound.test.tscovering stage transitions, single-report guarantees, ICEcheckingclearing the media timer, and give-up blocking furtherconnect()calls.Embedder note: consumers that only handle
KERNEL_CONNECTION_TIMEOUTwill miss newKERNEL_CONNECTION_FAILEDcases until updated (per PR dependency on kernel#4421).Reviewed by Cursor Bugbot for commit 9c222e9. Bugbot is set up for automated code reviews on this repo. Configure here.