From 950c79d641eaa0da3f81721c7f206b88dee73d63 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 13:05:39 -0400 Subject: [PATCH 1/3] fix(js): treat session notifications as hints and read the daemon's state loggedIn, loggedOut and HTTPSrvInfoUpdate are sent from separate goroutines and can reach the GUI out of order, so none of them sets the session any more. Each starts a fresh bootstrap-status read, and only the latest read's reply applies; a superseded read settles with the newer one. The session is also re-read after login RPCs return and when the network comes back. A reply for a different user while logged in logs out first, clearing the old account's stores. A logged-out reply during an account switch is still ignored, and is applied once the switch ends. Reachability tracking is dropped: the service is no longer asked to start it or send its notifications; a network change still nudges gregor through checkReachability. The http server address survives a logout. --- shared/constants/init/shared.test.ts | 220 ++++++++++++++++++++++++++- shared/constants/init/shared.tsx | 63 ++++++-- shared/constants/rpc/index.tsx | 3 +- shared/engine/index.platform.tsx | 3 +- shared/login/loading.tsx | 2 +- shared/provision/flow.tsx | 2 + shared/stores/config.tsx | 63 +------- shared/stores/daemon.tsx | 80 ++++++---- shared/stores/shell.tsx | 14 +- shared/stores/tests/config.test.ts | 80 ++++++++++ shared/stores/tests/daemon.test.ts | 169 ++++++++++++++++++++ 11 files changed, 591 insertions(+), 108 deletions(-) diff --git a/shared/constants/init/shared.test.ts b/shared/constants/init/shared.test.ts index e09a104c6ff6..d405de133345 100644 --- a/shared/constants/init/shared.test.ts +++ b/shared/constants/init/shared.test.ts @@ -2,8 +2,9 @@ import * as T from '@/constants/types' import {resetAllStores} from '@/util/zustand' import {useConfigState} from '@/stores/config' +import {useCurrentUserState} from '@/stores/current-user' import {useDaemonState} from '@/stores/daemon' -import {loadAccountsStep} from './shared' +import {_onEngineIncoming, initSharedSubscriptions, loadAccountsStep, onNetworkOnlineChanged} from './shared' describe('loadAccountsStep', () => { const originalDispatch = useConfigState.getState().dispatch @@ -65,3 +66,220 @@ describe('loadAccountsStep', () => { expect(useConfigState.getState().configuredAccounts.map(a => a.username)).toEqual(['testuser']) }) }) + +describe('onNetworkOnlineChanged', () => { + const originalDaemonDispatch = useDaemonState.getState().dispatch + afterEach(() => { + jest.restoreAllMocks() + useDaemonState.setState({dispatch: originalDaemonDispatch}) + resetAllStores() + }) + + const spyOnReRead = () => { + // userSwitching survives resetAllStores on purpose, and an earlier test in this file sets it + useConfigState.getState().dispatch.setUserSwitching(false) + const reRead = jest.fn() + useDaemonState.setState({ + dispatch: {...originalDaemonDispatch, refreshSessionFromDaemon: reRead}, + handshakeState: 'done', + }) + return reRead + } + + test('re-reads the session when the network comes back', () => { + const reRead = spyOnReRead() + onNetworkOnlineChanged(true, false) + expect(reRead).toHaveBeenCalledTimes(1) + }) + + test('does not re-read on the first reading of the network at startup', () => { + const reRead = spyOnReRead() + onNetworkOnlineChanged(true, undefined) + expect(reRead).not.toHaveBeenCalled() + }) + + test('does not re-read when going offline', () => { + const reRead = spyOnReRead() + onNetworkOnlineChanged(false, true) + expect(reRead).not.toHaveBeenCalled() + }) + + test('does not re-read during an account switch', () => { + const reRead = spyOnReRead() + useConfigState.getState().dispatch.setUserSwitching(true) + onNetworkOnlineChanged(true, false) + expect(reRead).not.toHaveBeenCalled() + }) + + test('does not re-read before the handshake is done', () => { + const reRead = spyOnReRead() + useDaemonState.setState({handshakeState: 'loading'}) + onNetworkOnlineChanged(true, false) + expect(reRead).not.toHaveBeenCalled() + }) +}) + +describe('the session comes from the daemon; notifications only say to read it', () => { + const status = (over: Partial = {}) => + ({ + deviceID: 'd1', + deviceName: 'testuser-mac', + fullname: '', + loggedIn: true, + registered: true, + uid: 'u1', + username: 'testuser', + ...over, + }) as unknown as T.RPCGen.BootstrapStatus + const userA = status() + const userB = status({deviceID: 'd2', uid: 'u2', username: 'testuser2'}) + const loggedOut = status({deviceID: '', deviceName: '', loggedIn: false, uid: '', username: ''}) + + let replies: Array<(bs: T.RPCGen.BootstrapStatus) => void> = [] + const flush = async () => jest.advanceTimersByTimeAsync(0) + const notify = (type: string, params: unknown) => _onEngineIncoming({payload: {params}, type} as never) + const readReplying = async (bs: T.RPCGen.BootstrapStatus) => { + useDaemonState.getState().dispatch.refreshSessionFromDaemon('test') + await flush() + replies[replies.length - 1]?.(bs) + await flush() + } + + // what resetAllStores clears, standing in for the previous account's state + const markAccountState = () => useConfigState.setState({justDeletedSelf: 'testuser'}) + const accountStateCleared = () => useConfigState.getState().justDeletedSelf === '' + const loginChanges = () => { + const changes: Array = [] + const unsub = useConfigState.subscribe((st, prev) => { + if (st.loggedIn !== prev.loggedIn) { + changes.push(st.loggedIn) + } + }) + return {changes, unsub} + } + + beforeEach(() => { + jest.useFakeTimers() + replies = [] + jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockImplementation( + async () => + new Promise(resolve => { + replies.push(resolve) + }) + ) + jest.spyOn(T.RPCGen, 'loginGetConfiguredAccountsRpcPromise').mockResolvedValue([]) + useConfigState.getState().dispatch.setUserSwitching(false) + initSharedSubscriptions() + }) + afterEach(() => { + jest.useRealTimers() + jest.restoreAllMocks() + useConfigState.getState().dispatch.setUserSwitching(false) + resetAllStores() + }) + + test('loggedOut and loggedIn delivered reversed still end with the last reply', async () => { + // the service logged out and then in; its two notifications reached us the other way round + notify('keybase.1.NotifySession.loggedIn', {signedUp: false, username: 'testuser'}) + notify('keybase.1.NotifySession.loggedOut', undefined) + await flush() + replies[1]?.(userA) + await flush() + replies[0]?.(loggedOut) + await flush() + + expect(useConfigState.getState().loggedIn).toBe(true) + expect(useCurrentUserState.getState().username).toBe('testuser') + }) + + test('a loggedOut hint logs out once the daemon says so', async () => { + await readReplying(userA) + expect(useConfigState.getState().loggedIn).toBe(true) + + notify('keybase.1.NotifySession.loggedOut', undefined) + expect(useConfigState.getState().loggedIn).toBe(true) + await flush() + replies[replies.length - 1]?.(loggedOut) + await flush() + + expect(useConfigState.getState().loggedIn).toBe(false) + }) + + test('an http server update applies at once and re-reads the daemon too', async () => { + const before = replies.length + notify('keybase.1.NotifyService.HTTPSrvInfoUpdate', {info: {address: '127.0.0.1:2', token: 'token'}}) + expect(useConfigState.getState().httpSrv.address).toBe('127.0.0.1:2') + await flush() + expect(replies.length).toBe(before + 1) + }) + + test('a reply for another user while logged in logs out first, clearing the old account', async () => { + await readReplying(userA) + markAccountState() + const {changes, unsub} = loginChanges() + + await readReplying(userB) + unsub() + + expect(changes).toEqual([false, true]) + expect(accountStateCleared()).toBe(true) + expect(useCurrentUserState.getState().uid).toBe('u2') + expect(useCurrentUserState.getState().username).toBe('testuser2') + expect(useDaemonState.getState().bootstrapStatus?.uid).toBe('u2') + }) + + test('the same user again is not a switch', async () => { + await readReplying(userA) + markAccountState() + const {changes, unsub} = loginChanges() + + await readReplying({...userA, fullname: 'changed'} as T.RPCGen.BootstrapStatus) + unsub() + + expect(changes).toEqual([]) + expect(accountStateCleared()).toBe(false) + }) + + test('logged in with no current user yet is not a switch', async () => { + useConfigState.getState().dispatch.setLoggedIn(true) + markAccountState() + const {changes, unsub} = loginChanges() + + await readReplying(userA) + unsub() + + expect(changes).toEqual([]) + expect(accountStateCleared()).toBe(false) + expect(useCurrentUserState.getState().uid).toBe('u1') + }) + + test('during an account switch a logged-out reply is ignored, and the new user still replaces the old', async () => { + await readReplying(userA) + markAccountState() + useConfigState.getState().dispatch.setUserSwitching(true) + const {changes, unsub} = loginChanges() + + await readReplying(loggedOut) + expect(useConfigState.getState().loggedIn).toBe(true) + + await readReplying(userB) + unsub() + + expect(changes).toEqual([false, true]) + expect(accountStateCleared()).toBe(true) + expect(useCurrentUserState.getState().username).toBe('testuser2') + expect(useConfigState.getState().userSwitching).toBe(true) + }) + + test('a switch whose login fails ends logged out, no longer switching', async () => { + await readReplying(userA) + useConfigState.getState().dispatch.setUserSwitching(true) + await readReplying(loggedOut) + + useConfigState.getState().dispatch.setLoginError(new Error('bad password') as never) + await readReplying(loggedOut) + + expect(useConfigState.getState().loggedIn).toBe(false) + expect(useConfigState.getState().userSwitching).toBe(false) + }) +}) diff --git a/shared/constants/init/shared.tsx b/shared/constants/init/shared.tsx index a338bd9d773b..78c2da8a671a 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -176,14 +176,15 @@ const onGregorPushStateChanged = ( ) } -const onGregorReachableChanged = (gregorReachable: ConfigState['gregorReachable']) => { - // Re-get info about our account if you log in/we're done handshaking/became reachable - if ( - gregorReachable === T.RPCGen.Reachable.yes && - useDaemonState.getState().handshakeState === 'done' && - !useConfigState.getState().userSwitching - ) { - ignorePromise(useDaemonState.getState().dispatch.loadDaemonBootstrapStatus()) +// After an offline stretch, reread the session to pick up what the service learned while we could +// not reach it. `previous === undefined` is the first reading of the network at startup, which the +// handshake's own read already covers. +export const onNetworkOnlineChanged = (online?: boolean, previous?: boolean) => { + if (!online || previous !== false) { + return + } + if (useDaemonState.getState().handshakeState === 'done' && !useConfigState.getState().userSwitching) { + useDaemonState.getState().dispatch.refreshSessionFromDaemon('back online') } } @@ -221,16 +222,30 @@ const onBootstrapStatusChanged = (bootstrap: DaemonState['bootstrapStatus']) => } const {deviceID, deviceName, loggedIn, uid, username} = bootstrap - useCurrentUserState.getState().dispatch.setBootstrap({deviceID, deviceName, uid, username}) - const configDispatch = useConfigState.getState().dispatch - if (username) { - configDispatch.setDefaultUsername(username) - } + + // Before the identity: the user we hold is what tells the new account's session from the old. + // onUserSwitchingChanged applies the status once the switch ends. if (!loggedIn && useConfigState.getState().userSwitching) { logger.info('[Bootstrap] ignoring loggedIn=false result during account switch') return } + + // Logged in as someone else than the user we hold is a logout and then a login, however the + // notifications in between reached us. Logging out clears the previous account's stores, the + // daemon's status among them, so put this status back and let that change apply it. + const currentUid = useCurrentUserState.getState().uid + if (loggedIn && useConfigState.getState().loggedIn && currentUid && uid !== currentUid) { + logger.info('[Bootstrap] the session is another user now, logging out the previous one') + configDispatch.setLoggedIn(false) + useDaemonState.getState().dispatch.setBootstrapStatus(bootstrap) + return + } + + useCurrentUserState.getState().dispatch.setBootstrap({deviceID, deviceName, uid, username}) + if (username) { + configDispatch.setDefaultUsername(username) + } configDispatch.setLoggedIn(loggedIn) if (bootstrap.httpSrvInfo) { @@ -238,6 +253,14 @@ const onBootstrapStatusChanged = (bootstrap: DaemonState['bootstrapStatus']) => } } +// A switch that failed after the service logged out has a logged-out status nothing applied, and +// a read after the switch returns the same status, which does not count as a change. +const onUserSwitchingChanged = (userSwitching: ConfigState['userSwitching']) => { + if (!userSwitching) { + onBootstrapStatusChanged(useDaemonState.getState().bootstrapStatus) + } +} + // Native reports the app state from the same callbacks that report it to Go, and this is the only // writer of mobileAppState. Desktop has no lifecycle; its window focus goes straight to appFocused. export const applyMobileAppState = (state: AppLifecycleState) => { @@ -325,7 +348,7 @@ export const onEngineConnected = () => { chatattachments: true, chatdev: false, chatemoji: false, chatemojicross: false, chatkbfsedits: false, deviceclone: false, ephemeral: false, favorites: false, featuredBots: false, kbfs: true, kbfsdesktop: !isMobile, devicehistory: true, kbfslegacy: false, kbfsrequest: false, kbfssubscription: true, keyfamily: false, notifysimplefs: true, - paperkeys: false, pgp: true, reachability: true, runtimestats: true, saltpack: true, service: true, session: true, + paperkeys: false, pgp: true, reachability: false, runtimestats: true, saltpack: true, service: true, session: true, team: true, teambot: false, tracking: true, users: true, wallet: false, }, }) @@ -361,14 +384,15 @@ export const initSharedSubscriptions = (platformBootstrapSteps: Array s.gregorReachable, onGregorReachableChanged), subscribeValue(useConfigState, s => s.gregorPushState, onGregorPushStateChanged), subscribeValue(useConfigState, s => s.loggedIn, onLoggedInChanged), subscribeValue(useConfigState, s => s.revokedTrigger, onRevokedTriggerChanged), - subscribeValue(useConfigState, s => s.configuredAccounts, onConfiguredAccountsChanged) + subscribeValue(useConfigState, s => s.configuredAccounts, onConfiguredAccountsChanged), + subscribeValue(useConfigState, s => s.userSwitching, onUserSwitchingChanged) ) _sharedUnsubs.push(subscribeValue(useDaemonState, s => s.bootstrapStatus, onBootstrapStatusChanged)) + _sharedUnsubs.push(subscribeValue(useShellState, s => s.networkStatus?.online, onNetworkOnlineChanged)) _sharedUnsubs.push( subscribeValue(useRouterState, s => s.navState, onNavStateChanged) @@ -388,6 +412,13 @@ export const _onEngineIncoming = (action: EngineGen.Actions) => { } switch (action.type) { + // These can reach us out of order with each other, so none of them sets the session: each only + // says it changed, and the daemon's reply to the latest read is what applies. + case 'keybase.1.NotifySession.loggedIn': + case 'keybase.1.NotifySession.loggedOut': + case 'keybase.1.NotifyService.HTTPSrvInfoUpdate': + useDaemonState.getState().dispatch.refreshSessionFromDaemon(action.type) + break case 'keybase.1.NotifyBadges.badgeState': { const {badgeState} = action.payload.params diff --git a/shared/constants/rpc/index.tsx b/shared/constants/rpc/index.tsx index 639df9e16838..fa83b6fa3e0e 100644 --- a/shared/constants/rpc/index.tsx +++ b/shared/constants/rpc/index.tsx @@ -78,8 +78,7 @@ type Keybase1IncomingAction = 'keybase.1.NotifyFS.FSActivity' | 'keybase.1.NotifySession.loggedOut' | 'keybase.1.NotifyTracking.trackingChanged' | - 'keybase.1.NotifyUsers.userChanged' | - 'keybase.1.reachability.reachabilityChanged' + 'keybase.1.NotifyUsers.userChanged' type Keybase1IncomingActionMap = { [P in K]: {readonly params: keybase1Types.RpcIn

} diff --git a/shared/engine/index.platform.tsx b/shared/engine/index.platform.tsx index f0cf37f0ba68..8e64b8c50219 100644 --- a/shared/engine/index.platform.tsx +++ b/shared/engine/index.platform.tsx @@ -257,7 +257,8 @@ function createClient( // from a session cancel handler inside disconnectCallback must // not strand the UI on the disconnect banner by skipping // connectCallback (which synchronously clears the daemon error - // via startHandshake()). + // via startHandshake(), so nothing here may be moved behind an + // await). client.transport.reset() try { disconnectCallback() diff --git a/shared/login/loading.tsx b/shared/login/loading.tsx index bac0b0886a50..a95e7da4b2c2 100644 --- a/shared/login/loading.tsx +++ b/shared/login/loading.tsx @@ -26,7 +26,7 @@ const SplashContainer = () => { C.Router2.navigateAppend({name: 'feedback', params: {}}) } : undefined - const onRetry = handshakeFailed ? startHandshake : undefined + const onRetry = handshakeFailed ? () => startHandshake() : undefined return } diff --git a/shared/provision/flow.tsx b/shared/provision/flow.tsx index 930227176203..f8d68c93ac73 100644 --- a/shared/provision/flow.tsx +++ b/shared/provision/flow.tsx @@ -9,6 +9,7 @@ import {ignorePromise, wrapErrors} from '@/constants/utils' import {type CommonResponseHandler} from '@/engine/types' import {callNamed, setNamedScoped} from '@/stores/flow-handles' import {useConfigState} from '@/stores/config' +import {useDaemonState} from '@/stores/daemon' import {useWaitingState} from '@/stores/waiting' import {RPCError} from '@/util/errors' @@ -421,6 +422,7 @@ const runProvision = (initialUsername: string) => { pauseRequested = false try { await runAttempt() + useDaemonState.getState().dispatch.refreshSessionFromDaemon('provision login returned') break } catch (_finalError) { if (wasUserCancelled()) { diff --git a/shared/stores/config.tsx b/shared/stores/config.tsx index 18715f9c7129..fb825ff34168 100644 --- a/shared/stores/config.tsx +++ b/shared/stores/config.tsx @@ -12,6 +12,7 @@ import {type CommonResponseHandler} from '@/engine/types' import {invalidPasswordErrorString} from '@/constants/config' import {navigateAppend} from '@/constants/router' import {onEngineConnected as onEngineConnectedInPlatform} from '@/util/storeless-actions' +import {useDaemonState} from '@/stores/daemon' type Store = T.Immutable<{ allowAnimatedEmojis: boolean @@ -24,7 +25,6 @@ type Store = T.Immutable<{ configuredAccounts: Array defaultUsername: string globalError?: Error | RPCError - gregorReachable?: T.RPCGen.Reachable gregorPushState: Array<{md: T.RPCGregor.Metadata; item: T.RPCGregor.Item}> loginError?: RPCError httpSrv: { @@ -62,7 +62,6 @@ const initialStore: Store = { defaultUsername: '', globalError: undefined, gregorPushState: [], - gregorReachable: undefined, httpSrv: { address: '', token: '', @@ -112,7 +111,6 @@ export type State = Store & { setChatStaticConfig: (s: T.Chat.StaticConfig) => void setDefaultUsername: (u: string) => void setGlobalError: (e?: unknown) => void - setGregorReachable: (r: Store['gregorReachable']) => void setHTTPSrvInfo: (address: string, token: string) => void setJustDeletedSelf: (s: string) => void setLoggedIn: (l: boolean) => void @@ -151,14 +149,6 @@ export const useConfigState = Z.createZustand('config', (set, get) => { } } - const setGregorReachable = (r: Store['gregorReachable']) => { - const old = get().gregorReachable - if (old === r) return - set(s => { - s.gregorReachable = r - }) - } - const setGregorPushState = (state: T.RPCGen.Gregor1.State) => { const items = state.items || [] const goodState = items.reduce>( @@ -278,18 +268,18 @@ export const useConfigState = Z.createZustand('config', (set, get) => { waitingKey: waitingKeyConfigLogin, }) logger.info('login call succeeded') - get().dispatch.setLoggedIn(true) } catch (error) { if (!(error instanceof RPCError)) { return } - if (error.code === T.RPCGen.StatusCode.scalreadyloggedin) { - get().dispatch.setLoggedIn(true) - } else if (error.desc !== cancelDesc) { - // If we're canceling then ignore the error + // Already logged in: the daemon's session says so. Canceling: nothing to report. + if (error.code !== T.RPCGen.StatusCode.scalreadyloggedin && error.desc !== cancelDesc) { error.desc = niceError(error) get().dispatch.setLoginError(error) } + } finally { + // After setLoginError, which ends a switch: a failed switch must apply a logged-out session. + useDaemonState.getState().dispatch.refreshSessionFromDaemon('login returned') } } get().dispatch.setLoginError() @@ -319,19 +309,6 @@ export const useConfigState = Z.createZustand('config', (set, get) => { // An engine reset drops in-flight RPCs without settling their promises; a refresh // caught by that would poison the dedupe cache forever inflightRefreshAccounts = undefined - // The startReachability RPC call both starts and returns the current - // reachability state. Then we'll get updates of changes from this state via reachabilityChanged. - // This should be run on app start and service re-connect in case the service somehow crashed or was restarted manually. - const startReachability = async () => { - try { - const reachability = await T.RPCGen.reachabilityStartReachabilityRpcPromise() - get().dispatch.setGregorReachable(reachability.reachable) - } catch (err) { - logger.warn('error bootstrapping reachability: ', err) - } - } - ignorePromise(startReachability()) - // If ever you want to get OOBMs for a different system, then you need to enter it here. const registerForGregorNotifications = async () => { try { @@ -375,29 +352,6 @@ export const useConfigState = Z.createZustand('config', (set, get) => { get().dispatch.setHTTPSrvInfo(action.payload.params.info.address, action.payload.params.info.token) break } - case 'keybase.1.NotifySession.loggedIn': { - logger.info('keybase.1.NotifySession.loggedIn') - // only send this if we think we're not logged in - const {loggedIn, dispatch} = get() - if (!loggedIn) { - dispatch.setLoggedIn(true) - } - break - } - case 'keybase.1.NotifySession.loggedOut': { - logger.info('keybase.1.NotifySession.loggedOut') - const {loggedIn, dispatch} = get() - // only send this if we think we're logged in (errors on provison can trigger this and mess things up) - if (loggedIn) { - dispatch.setLoggedIn(false) - } - break - } - case 'keybase.1.reachability.reachabilityChanged': - if (get().loggedIn) { - get().dispatch.setGregorReachable(action.payload.params.reachability.reachable) - } - break default: } }, @@ -459,6 +413,8 @@ export const useConfigState = Z.createZustand('config', (set, get) => { configuredAccounts: s.configuredAccounts, defaultUsername: s.defaultUsername, dispatch: s.dispatch, + // process-wide, not per account; nothing reloads it on logout + httpSrv: s.httpSrv, startup: {loaded: s.startup.loaded}, userSwitching: s.userSwitching, })) @@ -523,9 +479,6 @@ export const useConfigState = Z.createZustand('config', (set, get) => { }) } }, - setGregorReachable: r => { - setGregorReachable(r) - }, setHTTPSrvInfo: (address, token) => { set(s => { s.httpSrv.address = address diff --git a/shared/stores/daemon.tsx b/shared/stores/daemon.tsx index 31c4371d35ff..1a922a71108a 100644 --- a/shared/stores/daemon.tsx +++ b/shared/stores/daemon.tsx @@ -35,7 +35,10 @@ export type State = Store & { dispatch: { initBootstrapSteps: (steps: Array) => void loadDaemonBootstrapStatus: () => Promise + /** reads the session afresh, superseding any read in flight; for hints that it changed */ + refreshSessionFromDaemon: (reason: string) => void resetState: () => void + setBootstrapStatus: (bs: T.RPCGen.BootstrapStatus) => void setError: (e?: Error) => void startHandshake: () => void updateUserReacjis: (userReacjis: T.RPCGen.UserReacjis) => void @@ -49,48 +52,71 @@ export const useDaemonState = Z.createZustand('daemon', (set, get) => { // bumped on every startHandshake (engine reconnect, splash Reload) so a stale in-flight // run can't write results over a newer one let generation = 0 + // The latest read in flight. Only the latest read's reply is applied: a read asked for later + // reflects every session change the service had made by then. let inflightBootstrapStatus: Promise | undefined + let readSeq = 0 - const dispatch: State['dispatch'] = { - initBootstrapSteps: steps => { - bootstrapSteps = steps - }, - loadDaemonBootstrapStatus: async () => { - if (inflightBootstrapStatus) { + const readBootstrapStatus = async () => { + const gen = generation + const seq = ++readSeq + const f = async (): Promise => { + const bs = await T.RPCGen.configGetBootstrapStatusRpcPromise() + logger.info( + `[Bootstrap] loggedIn: ${bs.loggedIn ? 1 : 0} http: ${bs.httpSrvInfo ? bs.httpSrvInfo.address : 'none'}` + ) + // a newer handshake owns the store now; don't write a potentially older status over its load + if (gen !== generation) { + return + } + if (seq !== readSeq) { + // superseded: settle with the newer read, so a caller awaiting this one sees its status return inflightBootstrapStatus } - const gen = generation - const f = async () => { - const bs = await T.RPCGen.configGetBootstrapStatusRpcPromise() - logger.info( - `[Bootstrap] loggedIn: ${bs.loggedIn ? 1 : 0} http: ${bs.httpSrvInfo ? bs.httpSrvInfo.address : 'none'}` - ) - // a newer handshake owns the store now; don't write a potentially older status over its load - if (gen !== generation || isEqual(bs, get().bootstrapStatus)) { - return - } - set(s => { - s.bootstrapStatus = T.castDraft(bs) - }) + if (isEqual(bs, get().bootstrapStatus)) { + return } - const p = f() - inflightBootstrapStatus = p - try { - await p - } finally { - if (inflightBootstrapStatus === p) { - inflightBootstrapStatus = undefined - } + set(s => { + s.bootstrapStatus = T.castDraft(bs) + }) + } + const p = f().finally(() => { + if (inflightBootstrapStatus === p) { + inflightBootstrapStatus = undefined } + }) + inflightBootstrapStatus = p + return p + } + + const dispatch: State['dispatch'] = { + initBootstrapSteps: steps => { + bootstrapSteps = steps + }, + loadDaemonBootstrapStatus: async () => inflightBootstrapStatus ?? readBootstrapStatus(), + refreshSessionFromDaemon: reason => { + logger.info(`[Bootstrap] reading the session: ${reason}`) + readBootstrapStatus().catch((error: unknown) => { + logger.warn('[Bootstrap] reading the session failed:', error) + }) }, resetState: () => { set(s => ({ ...s, ...initialStore, dispatch: s.dispatch, + // Both track the connection, not the account, and the closure counter behind the + // generation keeps climbing across a reset: zeroing the copy here would make the live + // connection's own in-flight work look superseded by a logout that happened under it. + handshakeGeneration: s.handshakeGeneration, handshakeState: s.handshakeState, })) }, + setBootstrapStatus: bs => { + set(s => { + s.bootstrapStatus = T.castDraft(bs) + }) + }, setError: e => { if (e) { logger.error('Error (daemon):', e) diff --git a/shared/stores/shell.tsx b/shared/stores/shell.tsx index 7bb9237e8ee0..47c09f9e8dc0 100644 --- a/shared/stores/shell.tsx +++ b/shared/stores/shell.tsx @@ -6,7 +6,6 @@ import isEqual from 'lodash/isEqual' import logger from '@/logger' import {RPCError} from '@/util/errors' import {defaultUseNativeFrame} from '@/constants/platform' -import {useConfigState} from '@/stores/config' export type ConnectionType = NetInfo.NetInfoStateType | 'notavailable' @@ -156,11 +155,16 @@ export const useShellState = Z.createZustand('shell', (set, get) => { s.networkStatus.type = type } }) - const updateGregor = async () => { - const reachability = await T.RPCGen.reachabilityCheckReachabilityRpcPromise() - useConfigState.getState().dispatch.setGregorReachable(reachability.reachable) + // Not for the result: the service re-dials gregor inside this call and reconnects if the + // dial fails, which is what gets it off a dead connection after the network moves. + const nudgeGregor = async () => { + try { + await T.RPCGen.reachabilityCheckReachabilityRpcPromise() + } catch (error) { + logger.warn('failed to check gregor reachability: ', error) + } } - ignorePromise(updateGregor()) + ignorePromise(nudgeGregor()) const updateFS = async () => { if (isInit) return diff --git a/shared/stores/tests/config.test.ts b/shared/stores/tests/config.test.ts index e6595a24ca60..7db9e2ef1bf5 100644 --- a/shared/stores/tests/config.test.ts +++ b/shared/stores/tests/config.test.ts @@ -1,4 +1,7 @@ /// +import * as T from '../../constants/types' +import {RPCError} from '../../util/errors' +import {useDaemonState} from '../daemon' import {noConversationIDKey} from '../../constants/types/chat/common' import {useConfigState} from '../config' @@ -116,3 +119,80 @@ test('custom resetState preserves the fields config intentionally carries across expect(state.userSwitching).toBe(true) expect(state.globalError).toBeUndefined() }) + +test('logging out keeps the http server address: it belongs to the process, not the account', () => { + const {dispatch} = useConfigState.getState() + dispatch.onEngineIncoming({ + payload: {params: {info: {address: '127.0.0.1:2', token: 'token'}}}, + type: 'keybase.1.NotifyService.HTTPSrvInfoUpdate', + } as never) + + dispatch.resetState() + + expect(useConfigState.getState().httpSrv).toEqual({address: '127.0.0.1:2', token: 'token'}) +}) + +test('loggedIn and loggedOut notifications do not set the session themselves', () => { + const {dispatch} = useConfigState.getState() + dispatch.onEngineIncoming({ + payload: {params: {signedUp: false, username: 'testuser'}}, + type: 'keybase.1.NotifySession.loggedIn', + } as never) + expect(useConfigState.getState().loggedIn).toBe(false) + + dispatch.setLoggedIn(true) + dispatch.onEngineIncoming({payload: {params: undefined}, type: 'keybase.1.NotifySession.loggedOut'} as never) + expect(useConfigState.getState().loggedIn).toBe(true) + dispatch.setLoggedIn(false) +}) + +describe('login', () => { + const originalDaemonDispatch = useDaemonState.getState().dispatch + let refresh: jest.Mock + beforeEach(() => { + refresh = jest.fn() + useDaemonState.setState({dispatch: {...originalDaemonDispatch, refreshSessionFromDaemon: refresh}}) + }) + afterEach(() => { + jest.restoreAllMocks() + useDaemonState.setState({dispatch: originalDaemonDispatch}) + }) + + const flush = async () => new Promise(resolve => setImmediate(resolve)) + + test('reads the session from the daemon when the login succeeds', async () => { + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockResolvedValue(undefined) + useConfigState.getState().dispatch.login('testuser', 'password') + await flush() + + expect(refresh).toHaveBeenCalledTimes(1) + expect(useConfigState.getState().loggedIn).toBe(false) + expect(useConfigState.getState().loginError).toBeUndefined() + }) + + test('reads the session from the daemon when already logged in', async () => { + jest + .spyOn(T.RPCGen, 'loginLoginRpcListener') + .mockRejectedValue(new RPCError('already logged in', T.RPCGen.StatusCode.scalreadyloggedin)) + useConfigState.getState().dispatch.login('testuser', 'password') + await flush() + + expect(refresh).toHaveBeenCalledTimes(1) + expect(useConfigState.getState().loggedIn).toBe(false) + expect(useConfigState.getState().loginError).toBeUndefined() + }) + + test('a failed switch stops switching before it reads the session, so a logged-out reply applies', async () => { + jest + .spyOn(T.RPCGen, 'loginLoginRpcListener') + .mockRejectedValue(new RPCError('bad password', T.RPCGen.StatusCode.scgeneric)) + const switchingWhenRead: Array = [] + refresh.mockImplementation(() => switchingWhenRead.push(useConfigState.getState().userSwitching)) + useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.login('testuser', 'password') + await flush() + + expect(switchingWhenRead).toEqual([false]) + expect(useConfigState.getState().loginError).toBeDefined() + }) +}) diff --git a/shared/stores/tests/daemon.test.ts b/shared/stores/tests/daemon.test.ts index dbb1ad951b0d..bdf8a5b0d2c3 100644 --- a/shared/stores/tests/daemon.test.ts +++ b/shared/stores/tests/daemon.test.ts @@ -1,5 +1,6 @@ /// import * as T from '@/constants/types' +import logger from '@/logger' import {ignorePromise} from '@/constants/utils' import {maxHandshakeTries} from '@/constants/values' import {resetAllStores} from '@/util/zustand' @@ -147,3 +148,171 @@ describe('daemon store', () => { expect(store.getState().handshakeRetriesLeft).toBe(maxHandshakeTries) }) }) + +describe('reading the session from the daemon', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + afterEach(() => { + jest.useRealTimers() + jest.restoreAllMocks() + resetAllStores() + }) + + const deferredReads = () => { + const replies: Array<(bs: T.RPCGen.BootstrapStatus) => void> = [] + const spy = jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockImplementation( + async () => + new Promise(resolve => { + replies.push(resolve) + }) + ) + return {replies, spy} + } + + test('a refresh asks again even while a read is in flight', async () => { + const {spy} = deferredReads() + const {dispatch} = useDaemonState.getState() + + ignorePromise(dispatch.loadDaemonBootstrapStatus()) + dispatch.refreshSessionFromDaemon('test') + await jest.advanceTimersByTimeAsync(0) + + expect(spy).toHaveBeenCalledTimes(2) + }) + + test('of two overlapping refreshes, the older reply is dropped', async () => { + const {replies} = deferredReads() + const {dispatch} = useDaemonState.getState() + + dispatch.refreshSessionFromDaemon('first') + dispatch.refreshSessionFromDaemon('second') + await jest.advanceTimersByTimeAsync(0) + replies[1]?.({...bootstrapStatus, username: 'newer'}) + await jest.advanceTimersByTimeAsync(0) + replies[0]?.({...bootstrapStatus, username: 'older'}) + await jest.advanceTimersByTimeAsync(0) + + expect(useDaemonState.getState().bootstrapStatus?.username).toBe('newer') + }) + + test('an older reply that lands first is dropped too, the newer one is coming', async () => { + const {replies} = deferredReads() + const {dispatch} = useDaemonState.getState() + + dispatch.refreshSessionFromDaemon('first') + dispatch.refreshSessionFromDaemon('second') + await jest.advanceTimersByTimeAsync(0) + replies[0]?.({...bootstrapStatus, username: 'older'}) + await jest.advanceTimersByTimeAsync(0) + + expect(useDaemonState.getState().bootstrapStatus).toBeUndefined() + + replies[1]?.({...bootstrapStatus, username: 'newer'}) + await jest.advanceTimersByTimeAsync(0) + + expect(useDaemonState.getState().bootstrapStatus?.username).toBe('newer') + }) + + test('a load superseded by a refresh settles with the refresh, so the handshake sees the newer status', async () => { + const {replies} = deferredReads() + const {dispatch} = useDaemonState.getState() + const seen: Array = [] + dispatch.initBootstrapSteps([ + async () => { + seen.push(useDaemonState.getState().bootstrapStatus?.username) + return Promise.resolve() + }, + ]) + + dispatch.startHandshake() + await jest.advanceTimersByTimeAsync(0) + dispatch.refreshSessionFromDaemon('hint') + await jest.advanceTimersByTimeAsync(0) + replies[0]?.({...bootstrapStatus, username: 'older'}) + await jest.advanceTimersByTimeAsync(0) + + expect(useDaemonState.getState().handshakeState).toBe('loading') + + replies[1]?.({...bootstrapStatus, username: 'newer'}) + await jest.advanceTimersByTimeAsync(0) + + expect(seen).toEqual(['newer']) + expect(useDaemonState.getState().handshakeState).toBe('done') + }) + + test('a load started after a refresh joins it instead of asking again', async () => { + const {replies, spy} = deferredReads() + const {dispatch} = useDaemonState.getState() + + dispatch.refreshSessionFromDaemon('hint') + const joined = dispatch.loadDaemonBootstrapStatus() + await jest.advanceTimersByTimeAsync(0) + replies[0]?.(bootstrapStatus) + await joined + + expect(spy).toHaveBeenCalledTimes(1) + expect(useDaemonState.getState().bootstrapStatus?.username).toBe('testuser') + }) + + test('a failed refresh is logged, not thrown', async () => { + jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockRejectedValue(new Error('down')) + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => {}) + + useDaemonState.getState().dispatch.refreshSessionFromDaemon('hint') + await jest.advanceTimersByTimeAsync(0) + + expect(warn).toHaveBeenCalled() + }) +}) + +describe('a superseded read', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + afterEach(() => { + jest.useRealTimers() + jest.restoreAllMocks() + resetAllStores() + }) + + test('does not write its status over the newer load', async () => { + // a reconnect invalidates in-flight reads: the generation orders client attempts + let resolveLosing!: (bs: T.RPCGen.BootstrapStatus) => void + jest + .spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise') + .mockReturnValueOnce( + new Promise(resolve => { + resolveLosing = resolve + }) + ) + .mockResolvedValue(bootstrapStatus) + const {dispatch} = useDaemonState.getState() + dispatch.initBootstrapSteps([]) + + const losing = dispatch.loadDaemonBootstrapStatus() + dispatch.startHandshake() + await jest.advanceTimersByTimeAsync(0) + resolveLosing({...bootstrapStatus, username: 'stale'}) + await losing + await jest.advanceTimersByTimeAsync(0) + + expect(useDaemonState.getState().bootstrapStatus?.username).toBe('testuser') + }) +}) + +test('resetState keeps the handshake generation: it counts connections, not accounts', () => { + jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockResolvedValue(bootstrapStatus) + const {dispatch} = useDaemonState.getState() + dispatch.initBootstrapSteps([]) + dispatch.startHandshake() + dispatch.startHandshake() + const gen = useDaemonState.getState().handshakeGeneration + + dispatch.resetState() + + expect(gen).toBeGreaterThan(0) + expect(useDaemonState.getState().handshakeGeneration).toBe(gen) + jest.restoreAllMocks() + resetAllStores() +}) From 625db5884e2c07d87bc7d0237af2e5638ac6b3c7 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 15:02:17 -0400 Subject: [PATCH 2/3] fix(js): end a cancelled account switch so its logged-out status applies A cancelled login (the PromptNewDeviceName hand-off, or a non-RPC error) left userSwitching set, which withholds the logged-out session forever. Also drop the bootstrap re-read on loggedIn: only a bootstrap reply sets it now. --- shared/constants/init/shared.test.ts | 22 ++++++++++++++++++++ shared/constants/init/shared.tsx | 3 --- shared/stores/config.tsx | 6 +++++- shared/stores/tests/daemon.test.ts | 30 +++++++++++++--------------- 4 files changed, 41 insertions(+), 20 deletions(-) diff --git a/shared/constants/init/shared.test.ts b/shared/constants/init/shared.test.ts index d405de133345..35521a017c36 100644 --- a/shared/constants/init/shared.test.ts +++ b/shared/constants/init/shared.test.ts @@ -1,6 +1,7 @@ /// import * as T from '@/constants/types' import {resetAllStores} from '@/util/zustand' +import {RPCError} from '@/util/errors' import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' import {useDaemonState} from '@/stores/daemon' @@ -282,4 +283,25 @@ describe('the session comes from the daemon; notifications only say to read it', expect(useConfigState.getState().loggedIn).toBe(false) expect(useConfigState.getState().userSwitching).toBe(false) }) + + test.each([ + ['cancelled', new RPCError('Canceling RPC', T.RPCGen.StatusCode.scgeneric)], + ['ended by a non-RPC error', new Error('engine reset')], + ])('a switch whose login is %s ends logged out, no longer switching', async (_, error) => { + await readReplying(userA) + useConfigState.getState().dispatch.setUserSwitching(true) + await readReplying(loggedOut) + expect(useConfigState.getState().loggedIn).toBe(true) + + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockRejectedValue(error) + useConfigState.getState().dispatch.login('testuser2', 'password') + await flush() + + expect(useConfigState.getState().userSwitching).toBe(false) + expect(useConfigState.getState().loggedIn).toBe(false) + replies[replies.length - 1]?.(loggedOut) + await flush() + expect(useConfigState.getState().loggedIn).toBe(false) + expect(useConfigState.getState().loginError).toBeUndefined() + }) }) diff --git a/shared/constants/init/shared.tsx b/shared/constants/init/shared.tsx index 78c2da8a671a..e33370b3f90f 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -190,9 +190,6 @@ export const onNetworkOnlineChanged = (online?: boolean, previous?: boolean) => const onLoggedInChanged = (loggedIn: ConfigState['loggedIn']) => { if (loggedIn) { - // runtime login: refresh bootstrap status. During the handshake this is already in - // flight, and the store dedupes it. - ignorePromise(useDaemonState.getState().dispatch.loadDaemonBootstrapStatus()) scheduleStartupOrReloginWork() } else { clearSignupEmail() diff --git a/shared/stores/config.tsx b/shared/stores/config.tsx index fb825ff34168..27a6222851a9 100644 --- a/shared/stores/config.tsx +++ b/shared/stores/config.tsx @@ -269,6 +269,10 @@ export const useConfigState = Z.createZustand('config', (set, get) => { }) logger.info('login call succeeded') } catch (error) { + // Nothing else ends a cancelled switch, and the logged-out status it withheld applies only then + if (!(error instanceof RPCError) || error.desc === cancelDesc) { + get().dispatch.setUserSwitching(false) + } if (!(error instanceof RPCError)) { return } @@ -278,7 +282,7 @@ export const useConfigState = Z.createZustand('config', (set, get) => { get().dispatch.setLoginError(error) } } finally { - // After setLoginError, which ends a switch: a failed switch must apply a logged-out session. + // After the switch ends: a failed switch must apply a logged-out session. useDaemonState.getState().dispatch.refreshSessionFromDaemon('login returned') } } diff --git a/shared/stores/tests/daemon.test.ts b/shared/stores/tests/daemon.test.ts index bdf8a5b0d2c3..bf5a00998a8c 100644 --- a/shared/stores/tests/daemon.test.ts +++ b/shared/stores/tests/daemon.test.ts @@ -147,6 +147,20 @@ describe('daemon store', () => { expect(store.getState().handshakeFailedReason).toBe('') expect(store.getState().handshakeRetriesLeft).toBe(maxHandshakeTries) }) + + test('resetState keeps the handshake generation: it counts connections, not accounts', () => { + jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockResolvedValue(bootstrapStatus) + const {dispatch} = useDaemonState.getState() + dispatch.initBootstrapSteps([]) + dispatch.startHandshake() + dispatch.startHandshake() + const gen = useDaemonState.getState().handshakeGeneration + + dispatch.resetState() + + expect(gen).toBeGreaterThan(0) + expect(useDaemonState.getState().handshakeGeneration).toBe(gen) + }) }) describe('reading the session from the daemon', () => { @@ -300,19 +314,3 @@ describe('a superseded read', () => { expect(useDaemonState.getState().bootstrapStatus?.username).toBe('testuser') }) }) - -test('resetState keeps the handshake generation: it counts connections, not accounts', () => { - jest.spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise').mockResolvedValue(bootstrapStatus) - const {dispatch} = useDaemonState.getState() - dispatch.initBootstrapSteps([]) - dispatch.startHandshake() - dispatch.startHandshake() - const gen = useDaemonState.getState().handshakeGeneration - - dispatch.resetState() - - expect(gen).toBeGreaterThan(0) - expect(useDaemonState.getState().handshakeGeneration).toBe(gen) - jest.restoreAllMocks() - resetAllStores() -}) From 33ec7e11e4cf56e65bdf07178bc39b585143ecfd Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 23 Sep 2026 15:32:56 -0400 Subject: [PATCH 3/3] fix(js): an http server update doesn't re-read the session The payload carries the address and token and the config store applies it; only loggedIn/loggedOut are hints to re-read the daemon. --- shared/constants/init/shared.test.ts | 6 +++--- shared/constants/init/shared.tsx | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/shared/constants/init/shared.test.ts b/shared/constants/init/shared.test.ts index 35521a017c36..6ed968b2b26e 100644 --- a/shared/constants/init/shared.test.ts +++ b/shared/constants/init/shared.test.ts @@ -206,12 +206,12 @@ describe('the session comes from the daemon; notifications only say to read it', expect(useConfigState.getState().loggedIn).toBe(false) }) - test('an http server update applies at once and re-reads the daemon too', async () => { + test('an http server update applies at once without re-reading the daemon', async () => { const before = replies.length notify('keybase.1.NotifyService.HTTPSrvInfoUpdate', {info: {address: '127.0.0.1:2', token: 'token'}}) - expect(useConfigState.getState().httpSrv.address).toBe('127.0.0.1:2') + expect(useConfigState.getState().httpSrv).toEqual({address: '127.0.0.1:2', token: 'token'}) await flush() - expect(replies.length).toBe(before + 1) + expect(replies.length).toBe(before) }) test('a reply for another user while logged in logs out first, clearing the old account', async () => { diff --git a/shared/constants/init/shared.tsx b/shared/constants/init/shared.tsx index e33370b3f90f..92fb84309b96 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -413,7 +413,6 @@ export const _onEngineIncoming = (action: EngineGen.Actions) => { // says it changed, and the daemon's reply to the latest read is what applies. case 'keybase.1.NotifySession.loggedIn': case 'keybase.1.NotifySession.loggedOut': - case 'keybase.1.NotifyService.HTTPSrvInfoUpdate': useDaemonState.getState().dispatch.refreshSessionFromDaemon(action.type) break case 'keybase.1.NotifyBadges.badgeState':