Repository navigation
Reconnect the live view after a drop, and only say shut down when it is - #488
Draft
tnsardesai wants to merge 1 commit into
Draft
tnsardesai wants to merge 1 commit into
tnsardesai wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The live view showed "Browser has been shut down and is no longer available" whenever its connection dropped, whether or not the browser was gone.
app.vuerenders that overlay for any!connected && wasConnected, and once a connected session dropped, nothing reconnected it: ICE reachingfailed, the data channel closing, or the socket closing all ended indisconnect().So a network change or a TURN relay that stops answering told the viewer the browser had shut down while it was still running. Because the viewer no longer held a connection, the session then stopped counting as active and could idle out shortly after, which made the message come true.
Changes
4410when the browser session no longer exists. A reconnect that gets that code stops and shows the shut-down message. The overlay now has three states:4410): "Browser has been shut down and is no longer available"connection shutdownisn't a kick. neko sendssystem/disconnectwithconnection shutdownas it stops, both on a restart and on browser teardown. That message is now left to the socket close that follows, so it reconnects, or ends as gone, without a modal. Other server reasons (kicked,connection replaced, ...) stay terminal, so two viewers can't keep replacing each other.KERNEL_CONNECTION_FAILEDor, if the last attempt hit a stage bound,KERNEL_CONNECTION_TIMEOUT, with the last attempt'sreason. That keeps "a failed connect posts one of them, never both". A dropped session that comes back postsKERNEL_CONNECTEDandKERNEL_PLAYINGagain. A reconnect that ends as gone posts no failure, since it isn't a connect failure.Read-only mode survives a reconnect:
remote.reset()keepslocked = readOnly. A reconnect also re-runs the existingKERNEL_CONNECTEDhandling inapp.vue, which reapplies?width/height/ratequery params, the same as a page reload.The proxy appends the session token by rewriting
${e}?password=${encodeURIComponent(t)}&username=${encodeURIComponent(n)}in the bundle. I checked that the production build still contains that string exactly once, so reconnect sockets carry the token.Relation to #417's no-retry decision
#417 removed retries because retrying a connect that never succeeded delays the
reasonthe embedder needs. That still holds here: a first connect is not retried, and reports its first failure exactly as before.This PR reconnects only after a session that had connected drops. On
mainthat path posts no reason at all. The parent's only signal isKERNEL_PAUSED, which still arrives in the same tick as the drop, becausecleanup()runs before the reconnect is scheduled. So the reconnect delays nothing the parent receives. It adds one terminal event when the attempts run out, wheremainposted none.What the reconnect does delay is the in-viewer message when the browser really is gone:
4410On
mainall three show "Browser has been shut down" immediately, which in the last two is usually wrong.A reconnect that ends as gone posts nothing to the parent, the same as
main. The4410path was verified on a hypeman metro. The proxy has the same handling for the unikraft backend, but it was not run there.Testing
bun test tests: 262 pass. New tests:KERNEL_CONNECTION_FAILED;KERNEL_CONNECTION_TIMEOUT;4410close stops reconnecting without posting;connection shutdownversus other server disconnect reasons.The 7 reconnect tests fail against the previous
base.ts.npm run buildpasses, with the same 17 lint warnings asmain. Prettier is clean.End to end on a production headful image, with this branch's client and a local
mainbuild swapped into the same browser and driven from headless Chromium 155:mainsupervisorctl restart nekoreadOnly=truekept4410and showed "Browser has been shut down"; no further attemptsNot covered: the image build and a fresh boot with this client, and Safari or Firefox.
Follow-up
The live-view docs say "The client does not retry a connect itself" and that a viewer whose frames stop after connecting never posts a terminal event. Both need updating once this ships. The documented embedder pattern (arm a watchdog on
KERNEL_PAUSED, clear it onKERNEL_PLAYING) keeps working unchanged.