refactor(notify): one ordered per-connection clientState stream - #29653
Draft
chrisnojima wants to merge 12 commits into
Draft
chrisnojima wants to merge 12 commits into
chrisnojima wants to merge 12 commits into
Conversation
chrisnojima
added this pull request to stack #29658
September 21, 2026 18:56
chrisnojima
changed the title
This was referenced Sep 21, 2026
ClientSession and ClientState in notify_ctl, and a clientState oneway on NotifyApp that carries them. It rides the same ordered per-connection stream as the notifications that change those fields, so a client can apply everything in arrival order with no versions. The session is nullable on purpose: until the service's own startup login attempt settles there is no session to describe, and reporting a logged-out one would be a lie.
Every notification that a client applies as state -- loggedIn, loggedOut, HTTPSrvInfoUpdate, mobileAppStateChanged and the new clientState -- went out on its own goroutine, so two changes to the same field could arrive in either order. Give each connection an unbounded FIFO drained by one goroutine instead: queueing never blocks, and the rpc library writes one goroutine's Notify calls in the order they were made. loggedIn is a call, not a notification; callInOrder gives it the same place in line without holding the queue for the reply. NotifyRouter now owns the close listener it used to hand ConnectionManager, because it has a sender to stop as well as a slot to free, so ConnectionManager.AddConnection loses its closeListener parameter. ApplyAllDetails and ApplyDetailsFn go with HandleLogout's old body, their only caller. Shutdown stops the senders instead of doing nothing. NotifyRecorder is a test connection that records what it was sent in write order, which a real rpc.Server on the far end cannot do -- it serves each notification on its own goroutine.
lockSwitchUser wraps switchUserMu, the lock every session write is made under, and queues a clientState once the lock is released if the write changed the session. The promotion flag marks the writes a provisioning, signup or oneshot flow makes before it completes, so a client does not see the login early; the flow completes with SendLogin, which queues one. engine.SessionState factors the wait-free part of Bootstrap.Run out so the two cannot drift, and the service reads it, the http server address and the app state into a clientState when one is sent. The session is left out until the startup login attempt settles, and settling queues another clientState that carries it. A connection whose RegisterProtocols fails now signals its close channel, which frees its slot and its sender.
The bootstrap status, the loggedIn/loggedOut notifications and the login RPC's own reply were three sources for one fact, with no ordering between them. Take all of it from the clientState instead: it arrives on the service's ordered per-connection stream, so applyClientState applies whatever it carries in arrival order, and the bootstrap status keeps only the slower derived fields. A clientState with no session means the service's startup login attempt has not settled -- not logged out -- so the splash keeps waiting. sessionSettledStep makes that a handshake step, and subscribing moves out of onEngineConnected's fire-and-forget so the step can retry a failed subscribe. An account switch is a logout then a login, so a logged-in clientState for a different uid logs out first; that is what clears the previous account's stores. httpSrv survives the reset, being process-wide. Reachability goes with it: the service's gregor reachability was only ever a proxy for "the network came back", which shell already knows, so onNetworkOnlineChanged re-reads the bootstrap status instead and reachabilityChanged/startReachability leave the enabled calls.
…lients Init starts the loopback server before the login attempt settles, so a client can connect and subscribe while there is still no session. Its first clientState carries none, and the attempt settling sends another that does. The old comment described GetBootstrapStatus waiting for the attempt, which is no longer how a client learns the session.
…clientState The handshake waited only for a clientState carrying a session, and a service older than clientState never sends one. On Linux the GUI can be upgraded while such a service keeps running -- the post-install restart is skipped when autorestart is disabled, the service isn't under systemd, or the mount is busy -- and the splash then failed every retry until the user restarted it by hand. When the wait times out with no clientState at all on the connection, the session and http address now come from the bootstrap status the handshake already read. A clientState that arrived without a session still means the service's startup login hasn't settled, so that case keeps waiting.
Falling back to the bootstrap status covered only the startup handshake: with a service that never sends clientState, a later login, logout or account switch never reached the session, so the app was stuck in whatever state it started in. It also cost 30s on every connect, and a superseded connection's timer could settle the next connection's wait. Subscribing makes a current service send a clientState right away, so none at all by the deadline means a service older than the GUI. That is now a fatal handshake error telling the user to restart the service, rather than a mode that half works.
The mobile service runs in-process and is always the same build, so a clientState that is late there -- a stalled JS thread, a backed-up sender -- is not an out-of-date service. Only desktop, where the GUI can be upgraded under a running service, now fails the handshake for good.
A retry awaited the same subscription, so the service was never asked for another clientState: a lost one left every retry waiting its full 30s. Resetting the subscription on a timeout makes the retry subscribe, which is what sends a fresh clientState.
…n alone The timeout's subscription reset also ran for a step left over from a superseded connection, overwriting the subscription the new connection had just made, so its next retry subscribed a second time. The reset now applies only while the subscription is still the one the step waited on.
chrisnojima
force-pushed
the
nojima/HOTPOT-as-05-clientstate
branch
from
September 22, 2026 23:11
cdd4300 to
7dfb0b8
Compare
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
1 participant
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.
Stack 5/9, splitting #29637. Parent: #29652.
Why
Two related problems on master. First, notifications to a client are not ordered: each one is sent on its own goroutine, so two rapid changes can arrive in the wrong order and the client ends up with the older value. Second, the client learns the session by asking — reading bootstrap status — which races login and can show a stale logged-out state, and needs re-asking whenever something might have changed.
This replaces both with one ordered per-connection stream carrying a
ClientStatethe service pushes.Master bugs this fixes
Shutdowndoes not stop the senders, so notification goroutines can outlive the router.SetChannels.HandleLogoutreached into connection details viaApplyDetailsFn/ApplyAllDetails, machinery that existed for that one caller.What this changes
Each connection gets an unbounded FIFO drained by a single goroutine, so everything that connection is sent arrives in the order it was written.
Shutdownstops the senders;SetChannelsrefuses a dead connection;connmgrloses the details plumbing that only logout used.On top of that, the service announces a
ClientState— app state, session, http server info — once per connection at subscribe time and again on every session change.lockSwitchUsersnapshots the session identity around a switch and announces when it actually changed.engine.SessionStateis extracted fromBootstrap.Runas a pure move so both paths produce the same value. JS takes the session and the http address from that stream instead of re-reading bootstrap status.Why reachability is removed here and not earlier
reachability.reachabilityChanged/startReachabilitycome out ofenabled-calls.jsontogether with the JS that used them. They could not be removed in an earlier PR: the protocol regen deletes them fromrpc-gen.tsx, and master'srpc/index.tsxandstores/config.tsxstill reference them, so any earlier removal breaksyarn tsc. Removing the generated entries and their consumers in one commit is the only ordering that compiles.Worth a decision:
onGregorReachableChangedfired on every gregor connect/disconnect; its replacement fires only on a device-level offline→online edge. A gregor drop and re-dial while the device stays online no longer re-reads anything. This matches #29637 —AnnounceClientStateis called only fromlockSwitchUser, the login settle, and the logout/login/httpsrv announces — so it is not introduced here, but it is a deliberate deletion of a recovery trigger.A service that never sends a clientState
The splash waits for a clientState that carries a session. A service older than
ClientStatenever sends one. On Linux that can happen: the GUI gets upgraded while the old service keeps running, because the post-install restart is skipped when autorestart is disabled, the service isn't under systemd, or the mount is busy.FatalHandshakeErrortelling the user to restart the service (run_keybaseon Linux) and stops retrying. Subscribing makes a current service send a clientState right away, so none at all means an old service.I first tried falling back to
getBootstrapStatus, and dropped it. It only covered the startup handshake: with such a service, later logins, logouts and account switches never reached the session, so the app stayed stuck in whatever state it started in.Also from review:
Verification
go build,go vet,gofmt,golangci-lint --new-from-rev master0 issues. Notify and client-state tests pass under-raceat high repeat counts.yarn lint:allclean, unit suite green.Protocol artifacts were checked by running the real generators (
avdl2json,avdlc,generate-ts.ts) into scratch dirs and diffing — byte-identical to a true regeneration.Restores the two
appstate_test.gotests and twoapp-state.test.tscases #29651 had to hold back.Not verified here:
engine.SessionStateagainst a real logged-in bootstrap — those tests need kbweb onlocalhost:3000, unavailable in this environment. It is covered as a pure extraction plus logged-out service tests.