Skip to content

Surface and bound live view connect failures - #403

Open
robertjamesprior wants to merge 2 commits into
mainfrom
hypeship/bound-live-view-connect-retries
Open

robertjamesprior wants to merge 2 commits into
mainfrom
hypeship/bound-live-view-connect-retries

Conversation

@robertjamesprior

@robertjamesprior robertjamesprior commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Draft for review — this is the minimum needed to stop the live view failing silently, split out from the larger recovery work so it can land on its own.

onMessage is assigned directly to ws.onmessage, so a throw from createPeer or setRemoteOffer had nowhere to go. The rejection was discarded: no emit('error'), no onDisconnected, no overlay, no parent-frame message. The signaling socket stayed open and healthy, so the client could not distinguish "peer construction failed" from "still connecting". The only thing still running was the 15s connect watchdog, which closed the socket and reconnected into the identical failure — and because cleanup() re-renders the connect component, which auto-logs-in on mount, that cycle repeated indefinitely.

Three changes:

  • Route the throw. onMessage now wraps the handler so a rejection reaches onDisconnected. This covers the whole handler rather than just the signal/provide branch, since setRemoteAnswer and the rest can throw the same way.
  • Bound the retries. Consecutive attempts that never reach a connected peer are capped at 3, counted in connect() and reset in onConnected() once frames can actually flow. A retry that reproduces the same failure is not recovery.
  • Report the terminal state. On giving up, post KERNEL_CONNECTION_FAILED to the parent frame with the reason, attempt count, and the ICE/signaling/socket state at that moment, reusing the existing postParentMessage helper.

An unsupported browser (RTCPeerConnection missing) is now terminal as well. It previously called onDisconnected and then looped through the same auto-login cycle, which could never succeed.

Follow-ups this does not do

  • KERNEL_CONNECTION_FAILED is a new parent-frame event and is not yet documented. The live view events table was published in Document live view parent frame events docs#611, so that page needs a follow-up entry. The event name is worth a second opinion before it becomes a public contract.
  • No terminal UI. The user gets the existing disconnected notification with the reason. A dedicated terminal state with one-click resume, and sharing one recovery heuristic between the dashboard and embedded paths, are deliberately left out of this PR.
  • addIceCandidate is still not awaited in the candidate branch, so it remains a separate un-surfaced rejection path. Left alone here to keep the diff to the reported failure.
  • The underlying trigger — what made peer construction throw in the first place — is still unidentified. This change is worth making regardless of what that turns out to be.

Testing

bun test tests in images/chromium-headful/client: 27 pass, 0 fail (22 pre-existing plus 5 new in tests/connect-bound.test.ts, covering the swallowed throw, the attempt bound, the parent-frame payload, no further sockets after giving up, unsupported-browser termination, and the counter reset on success).

vue-cli-service lint reports no new warnings on the changed files.

Not yet exercised against a real browser image — the failure needs RTCPeerConnection to throw, which the test reproduces by construction.

🤖 Generated with Claude Code


Note

Medium Risk
Changes embedded live-view connection and recovery behavior and introduces a new parent-frame KERNEL_CONNECTION_FAILED contract; mis-handling could affect dashboards that embed the client.

Overview
Stops the chromium-headful live view from failing silently when WebRTC setup throws or never reaches a connected peer.

WebSocket handler errors are no longer swallowed: onMessage delegates to handleMessage inside try/catch so async failures from createPeer, setRemoteOffer, and related signaling paths flow into onDisconnected instead of leaving a healthy signaling socket with no peer.

Retries are capped at three connect attempts (_connectAttempts, reset in onConnected once media can flow). After that—or immediately when RTCPeerConnection is unsupported—the client sets _gaveUp, stops opening new sockets, and calls giveUp, which posts KERNEL_CONNECTION_FAILED to the parent frame (reason, attempt count, ICE/connection/signaling state, socket open) via the existing postParentMessage helper. Users still see the normal disconnected notification; there is no new terminal UI in this PR.

Adds tests/connect-bound.test.ts (five cases) for the swallowed-throw path, attempt bound, parent payload, no further connects after give-up, unsupported-browser termination, and counter reset on success.

Reviewed by Cursor Bugbot for commit f00dbc7. Bugbot is set up for automated code reviews on this repo. Configure here.

robertjamesprior and others added 2 commits September 18, 2026 23:39
onMessage is assigned directly to ws.onmessage, so a throw from createPeer
or setRemoteOffer became a discarded rejection: no error, no overlay, no
parent-frame message. The only thing left running was the 15s connect
watchdog, which closed the socket and reconnected into the identical failure
indefinitely.

Route a throw to onDisconnected, cap consecutive attempts that never reach a
connected peer, and post the terminal reason to the parent frame so embedders
can react. An unsupported browser is now terminal too rather than retried.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bound check incremented before comparing, so giving up after three
attempts reported four, contradicting its own message. Count only
attempts that opened a socket.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@robertjamesprior
robertjamesprior marked this pull request as ready for review September 19, 2026 00:20
@robertjamesprior

Copy link
Copy Markdown
Contributor Author

Out of draft. Verified before flipping it:

No dependencies. Single commit on main, not behind it, MERGEABLE. postParentMessage is already on main (added in #362, merged 09-14), so nothing here waits on an unmerged change. The only reconnect entry point is index.ts:61BaseClient.connect(), which the _gaveUp guard sits in front of, so there is no path that resumes the loop after giving up.

One accuracy fix pushed (f00dbc7). The bound check incremented before comparing:

if (++this._connectAttempts > MAX_CONNECT_ATTEMPTS) {

so on the call that gave up, attempts reported 4 while the message in the same payload said "did not start after 3 attempts". Only three sockets were ever opened; the fourth call was rejected before opening one. Now it compares first and increments after, so both agree at 3 and the field means what it says.

Worth catching now rather than later: KERNEL_CONNECTION_FAILED is a new event and is not yet in the published events table (kernel/docs#611), and embedders are actively being pointed at that table. Correcting the payload is free today and a breaking change once it ships.

All 5 tests pass locally (bun test tests/connect-bound.test.ts), and CI was green on the previous commit.

Note on scope: this stops the infinite loop and reports the terminal reason. The designed terminal state with one-click resume, the shared recovery heuristic, and the KERNEL_PLAYING gate are the remaining half, now folded back into KERNEL-2097 rather than tracked separately.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f00dbc7. Configure here.

} catch (err: unknown) {
this.onDisconnected(err instanceof Error ? err : new Error(String(err)))
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed connects skip parent report

High Severity

A throw from createPeer now reaches onDisconnected, which tears down the socket and clears the 15s watchdog, but never calls giveUp. connect() is only invoked from the connect overlay mounted hook, and that overlay stays mounted while connected remains false, so a later connect() never runs. KERNEL_CONNECTION_FAILED is never posted, and the parent also loses the former KERNEL_CONNECTION_TIMEOUT signal.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f00dbc7. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant