diff --git a/shared/constants/init/shared.test.ts b/shared/constants/init/shared.test.ts index e09a104c6ff6..6ed968b2b26e 100644 --- a/shared/constants/init/shared.test.ts +++ b/shared/constants/init/shared.test.ts @@ -1,9 +1,11 @@ /// 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' -import {loadAccountsStep} from './shared' +import {_onEngineIncoming, initSharedSubscriptions, loadAccountsStep, onNetworkOnlineChanged} from './shared' describe('loadAccountsStep', () => { const originalDispatch = useConfigState.getState().dispatch @@ -65,3 +67,241 @@ 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 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).toEqual({address: '127.0.0.1:2', token: 'token'}) + await flush() + expect(replies.length).toBe(before) + }) + + 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) + }) + + 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 a338bd9d773b..92fb84309b96 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -176,22 +176,20 @@ 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') } } 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() @@ -221,16 +219,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 +250,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 +345,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 +381,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 +409,12 @@ 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': + 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..27a6222851a9 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,22 @@ export const useConfigState = Z.createZustand('config', (set, get) => { waitingKey: waitingKeyConfigLogin, }) logger.info('login call succeeded') - get().dispatch.setLoggedIn(true) } 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 } - 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 the switch ends: a failed switch must apply a logged-out session. + useDaemonState.getState().dispatch.refreshSessionFromDaemon('login returned') } } get().dispatch.setLoginError() @@ -319,19 +313,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 +356,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 +417,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 +483,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..bf5a00998a8c 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' @@ -146,4 +147,170 @@ 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', () => { + 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') + }) })