From c3a69a0b793f6c8ab561034689e7592af65fc8f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sat, 19 Sep 2026 09:31:10 +0000 Subject: [PATCH] fix(mobile): retry transient keychain reads so the profile and language survive backgrounding Surface: the mobile app (apps/mobile). Two defects the owner sees on a real device. Find the ONE cause if they share it, and fix both. 1. After the app has been backgrounded for a while, the profile screen fails to load when the app comes back to the foreground. A retry loads it. 2. The chosen language occasionally reverts to the device language. Restarting the app restores the chosen language. Reproduce both first. Background the app long enough for the OS to release resources, then foreground it and open the profile; repeat until the failure shows. For the language, set a language that differs from the device language, background and foreground the app several times, and watch for the revert. The owner's hunch is the local database: a handle that does not survive the background, a read that runs before the store is open, or a write that never lands. Test that hunch first, and say in the pull request what the cause actually was. Required behaviour after the fix: - Foregrounding after any background period loads the profile on the first try, with no manual retry. - The chosen language survives backgrounding, foregrounding and a cold start, and only the user changes it. - Whatever the shared cause is, the fix closes it once rather than patching each screen. Cover both with a test that fails without the fix, and prove each on a device with a capture of the screen after the foreground. --- apps/mobile/src/lib/auth/credentials.test.ts | 54 ++++++++++++++++- apps/mobile/src/lib/auth/credentials.ts | 3 +- apps/mobile/src/lib/auth/token-owner.test.ts | 17 ++++++ apps/mobile/src/lib/auth/token-owner.ts | 5 +- .../lib/hooks/secure-store-preference.test.ts | 60 ++++++++++++++++--- .../src/lib/hooks/secure-store-preference.ts | 4 +- .../src/lib/persist/encrypted-kv.test.ts | 18 ++++++ apps/mobile/src/lib/persist/encrypted-kv.ts | 6 +- .../mobile/src/lib/persist/read-cache.test.ts | 18 ++++++ apps/mobile/src/lib/persist/read-cache.ts | 10 +++- apps/mobile/src/lib/trpc.test.ts | 6 -- apps/mobile/vitest.mounted.config.ts | 3 + apps/mobile/vitest.pure.config.ts | 3 + apps/mobile/vitest.setup.ts | 11 ++++ 14 files changed, 191 insertions(+), 27 deletions(-) create mode 100644 apps/mobile/vitest.setup.ts diff --git a/apps/mobile/src/lib/auth/credentials.test.ts b/apps/mobile/src/lib/auth/credentials.test.ts index 364c9f6711..94ce7612c7 100644 --- a/apps/mobile/src/lib/auth/credentials.test.ts +++ b/apps/mobile/src/lib/auth/credentials.test.ts @@ -27,8 +27,6 @@ vi.mock('expo-secure-store', () => ({ }), })); -vi.mock('@/lib/config', () => ({ API_BASE_URL: 'https://api.example.com' })); - // The sign-out deletes live in auth-context.tsx; mounting it pulls in the full // teardown graph, so stub every side-effecting collaborator. vi.mock('@sentry/react-native', () => ({ @@ -143,7 +141,7 @@ vi.mock('@/lib/temp-file-registry', () => ({ })); import * as SecureStore from 'expo-secure-store'; -import { persistSignInCredentialsAtEpoch } from '@/lib/auth/credentials'; +import { performRefresh, persistSignInCredentialsAtEpoch } from '@/lib/auth/credentials'; import { bumpAuthEpoch } from '@/lib/auth/auth-epoch'; import { clearActiveToken, setSignOutTeardownActive } from '@/lib/auth/token-owner'; import { @@ -214,6 +212,56 @@ describe('bearer credential writes', () => { }); }); +describe('refresh rotation', () => { + beforeEach(() => { + vi.clearAllMocks(); + store.clear(); + clearActiveToken(); + setSignOutTeardownActive(false); + }); + + it('retries a rejected refresh-token read and still rotates the token', async () => { + store.set(REFRESH_TOKEN_KEY, 'r1'); + // The keychain rejects the first read — the transient class on a device + // that just foregrounded — and resolves the stored value on the retry. + let reads = 0; + vi.mocked(SecureStore.getItemAsync).mockImplementation(async (key: string) => { + await Promise.resolve(); + reads += 1; + if (reads === 1) { + throw new Error('keychain temporarily unavailable'); + } + return store.get(key) ?? null; + }); + const fetchMock = vi + .fn() + .mockResolvedValue( + Response.json({ token: 't2', refreshToken: 'r2', expiresIn: 3600 }, { status: 200 }) + ); + vi.stubGlobal('fetch', fetchMock); + + try { + const outcome = await performRefresh(); + + expect(outcome.ok).toBe(true); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(SecureStore.setItemAsync).toHaveBeenCalledWith(AUTH_TOKEN_KEY, 't2', expectedOptions); + expect(SecureStore.setItemAsync).toHaveBeenCalledWith( + REFRESH_TOKEN_KEY, + 'r2', + expectedOptions + ); + expect(SecureStore.setItemAsync).toHaveBeenCalledWith( + TOKEN_EXPIRES_AT_KEY, + expect.any(String), + expectedOptions + ); + } finally { + vi.unstubAllGlobals(); + } + }); +}); + describe('sign-out deletes', () => { // mountAndSignOut resets modules and imports the whole auth graph; under the // full related-suite running concurrently with the device stack that exceeds diff --git a/apps/mobile/src/lib/auth/credentials.ts b/apps/mobile/src/lib/auth/credentials.ts index ef6af237b8..57faa2e10a 100644 --- a/apps/mobile/src/lib/auth/credentials.ts +++ b/apps/mobile/src/lib/auth/credentials.ts @@ -3,6 +3,7 @@ import * as SecureStore from 'expo-secure-store'; import { API_BASE_URL } from '@/lib/config'; import { currentAuthEpoch, isCurrentAuthEpoch } from '@/lib/auth/auth-epoch'; import { parseTokenPair } from '@/lib/auth/native-auth-contract'; +import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; import { isSignOutTeardownActive, setActiveToken } from '@/lib/auth/token-owner'; import { chainSave } from '@/lib/hooks/save-chain'; import { AUTH_TOKEN_KEY, REFRESH_TOKEN_KEY, TOKEN_EXPIRES_AT_KEY } from '@/lib/storage-keys'; @@ -146,7 +147,7 @@ async function doRefresh(): Promise { return { ok: false, refused: false, superseded: true }; } try { - const storedRefreshToken = await SecureStore.getItemAsync(REFRESH_TOKEN_KEY); + const storedRefreshToken = await readStoredValueWithRetry(REFRESH_TOKEN_KEY); if (superseded()) { return { ok: false, refused: false, superseded: true }; } diff --git a/apps/mobile/src/lib/auth/token-owner.test.ts b/apps/mobile/src/lib/auth/token-owner.test.ts index 314e215f63..e9acb18477 100644 --- a/apps/mobile/src/lib/auth/token-owner.test.ts +++ b/apps/mobile/src/lib/auth/token-owner.test.ts @@ -87,6 +87,23 @@ describe('token-owner', () => { expect(SecureStore.getItemAsync).toHaveBeenCalledTimes(1); }); + it('retries a rejected cold read and serves the stored token', async () => { + store.set(AUTH_TOKEN_KEY, 'stored-token'); + // The keychain rejects the first read — the transient class on a device + // that just foregrounded — and resolves the stored value on the retry. + let reads = 0; + vi.mocked(SecureStore.getItemAsync).mockImplementation(async (key: string) => { + await Promise.resolve(); + reads += 1; + if (reads === 1) { + throw new Error('keychain temporarily unavailable'); + } + return store.get(key) ?? null; + }); + + await expect(getAuthTokenForRequest()).resolves.toBe('stored-token'); + }); + it('warms the owner on a cold read so the next read is in-memory', async () => { store.set(AUTH_TOKEN_KEY, 'stored-token'); await getAuthTokenForRequest(); diff --git a/apps/mobile/src/lib/auth/token-owner.ts b/apps/mobile/src/lib/auth/token-owner.ts index 882e039b62..846b6f8b2b 100644 --- a/apps/mobile/src/lib/auth/token-owner.ts +++ b/apps/mobile/src/lib/auth/token-owner.ts @@ -1,6 +1,5 @@ -import * as SecureStore from 'expo-secure-store'; - import { currentAuthEpoch, isCurrentAuthEpoch } from '@/lib/auth/auth-epoch'; +import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; import { AUTH_TOKEN_KEY } from '@/lib/storage-keys'; export type ActiveToken = { @@ -97,7 +96,7 @@ export async function getAuthTokenForRequest(): Promise { if (isSignOutTeardownActive()) { return null; } - const token = await SecureStore.getItemAsync(AUTH_TOKEN_KEY); + const token = await readStoredValueWithRetry(AUTH_TOKEN_KEY); // A sign-in or refresh may have published a newer owner while the cold read // was in flight: prefer it and never overwrite it with the stale read. const published = getActiveToken(); diff --git a/apps/mobile/src/lib/hooks/secure-store-preference.test.ts b/apps/mobile/src/lib/hooks/secure-store-preference.test.ts index 8332f9c110..903a61091a 100644 --- a/apps/mobile/src/lib/hooks/secure-store-preference.test.ts +++ b/apps/mobile/src/lib/hooks/secure-store-preference.test.ts @@ -15,6 +15,14 @@ vi.mock('@sentry/react-native', () => ({ captureException })); const { toastError } = vi.hoisted(() => ({ toastError: vi.fn() })); vi.mock('sonner-native', () => ({ toast: { error: toastError } })); +// The retrying read imports @/lib/config, whose real module needs the app's +// baked build `extra` and cannot load under Vitest. Vitest also rejects a mock +// factory that omits an export the importer reads, so this stub answers the +// whole surface: every value reads as unset, which keeps the retry helper's +// fault window closed and leaves the failure to the SecureStore mock. +const configStub = vi.hoisted(() => new Proxy({}, { get: () => undefined, has: () => true })); +vi.mock('@/lib/config', () => configStub); + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule function flushMicrotasks(): Promise { return new Promise(resolve => { @@ -25,6 +33,10 @@ function flushMicrotasks(): Promise { // eslint-disable-next-line no-empty-function -- listener body is irrelevant, only subscribe()'s side effect (starting the load) is under test function noopListener(): void {} +// The retrying read backs off 250/500/1000 ms, so a test that drives a +// rejection must let those timers fire before the read settles. +const RETRY_BUDGET_MS = 250 + 500 + 1000 + 250; + describe('createSecureStorePreference', () => { beforeEach(() => { getItemAsync.mockReset(); @@ -35,6 +47,8 @@ describe('createSecureStorePreference', () => { }); it('logs to Sentry (not a toast) on a read failure and keeps the default value', async () => { + // Every attempt rejects, so the retry budget is exhausted before the + // failure is surfaced. getItemAsync.mockRejectedValue(new Error('disk error')); const store = createSecureStorePreference({ key: 'k', @@ -43,16 +57,46 @@ describe('createSecureStorePreference', () => { serialize: value => (value ? 'true' : 'false'), }); - const unsubscribe = store.subscribe(noopListener); - await flushMicrotasks(); + vi.useFakeTimers(); + try { + const unsubscribe = store.subscribe(noopListener); + await vi.advanceTimersByTimeAsync(RETRY_BUDGET_MS); + + expect(store.get()).toBe(false); + expect(store.getHasLoaded()).toBe(true); + expect(getItemAsync).toHaveBeenCalledTimes(4); + expect(captureException).toHaveBeenCalledWith(expect.any(Error), { + tags: { 'error.subsystem': 'preferences', 'error.operation': 'load_secure_store' }, + }); + expect(toastError).not.toHaveBeenCalled(); + unsubscribe(); + } finally { + vi.useRealTimers(); + } + }); - expect(store.get()).toBe(false); - expect(store.getHasLoaded()).toBe(true); - expect(captureException).toHaveBeenCalledWith(expect.any(Error), { - tags: { 'error.subsystem': 'preferences', 'error.operation': 'load_secure_store' }, + it('retries a rejected read and applies the value from the retry', async () => { + getItemAsync + .mockRejectedValueOnce(new Error('keychain unavailable')) + .mockResolvedValueOnce('true'); + const store = createSecureStorePreference({ + key: 'k', + defaultValue: false, + parse: raw => raw === 'true', + serialize: value => (value ? 'true' : 'false'), }); - expect(toastError).not.toHaveBeenCalled(); - unsubscribe(); + + vi.useFakeTimers(); + try { + const unsubscribe = store.subscribe(noopListener); + await vi.advanceTimersByTimeAsync(RETRY_BUDGET_MS); + + expect(store.get()).toBe(true); + expect(store.getHasLoaded()).toBe(true); + unsubscribe(); + } finally { + vi.useRealTimers(); + } }); it('shows a toast on a write failure while keeping the in-memory value', async () => { diff --git a/apps/mobile/src/lib/hooks/secure-store-preference.ts b/apps/mobile/src/lib/hooks/secure-store-preference.ts index 245442eaa5..f2ebb6d1e1 100644 --- a/apps/mobile/src/lib/hooks/secure-store-preference.ts +++ b/apps/mobile/src/lib/hooks/secure-store-preference.ts @@ -1,9 +1,9 @@ import * as Sentry from '@sentry/react-native'; -import * as SecureStore from 'expo-secure-store'; import { toast } from 'sonner-native'; import { i18n } from '@/i18n'; import { deleteAccountMetadata, setAccountMetadata } from '@/lib/auth/account-metadata-write'; +import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; function noop(): void { // Placeholder until the promise executor hands over its resolve. @@ -54,7 +54,7 @@ export function createSecureStorePreference(options: { const load = async () => { try { - const raw = await SecureStore.getItemAsync(key); + const raw = await readStoredValueWithRetry(key); if (!dirty) { value = parse(raw); } else if (mergeOnLoad && !cleared) { diff --git a/apps/mobile/src/lib/persist/encrypted-kv.test.ts b/apps/mobile/src/lib/persist/encrypted-kv.test.ts index 953c310210..c8bded4200 100644 --- a/apps/mobile/src/lib/persist/encrypted-kv.test.ts +++ b/apps/mobile/src/lib/persist/encrypted-kv.test.ts @@ -185,6 +185,7 @@ vi.mock('@sentry/react-native', () => ({ })); /* eslint-disable import/first */ +import * as SecureStore from 'expo-secure-store'; import * as SQLite from 'expo-sqlite'; import { PERSIST_DB_KEY } from '@/lib/storage-keys'; import { @@ -363,6 +364,23 @@ describe('single-flight open contract', () => { }); }); +describe('transient keychain read', () => { + it('retries a rejected key read instead of memoizing the open failure', async () => { + // The keychain read can reject transiently (device just rebooted, keystore + // still locked). The open must retry it: a memoized rejection here would + // reject every KV caller for the rest of the process. + const storedKey = 'a'.repeat(64); + store.set(PERSIST_DB_KEY, storedKey); + vi.mocked(SecureStore.getItemAsync).mockRejectedValueOnce(new Error('keychain unavailable')); + + await expect(getItem('s', 'k')).resolves.toBeNull(); + + // The retry read the stored key and the store opened on it. + expect(SQLite.openDatabaseSync).toHaveBeenCalledTimes(1); + expect(sqlLog).toContain(`PRAGMA key = "x'${storedKey}'"`); + }); +}); + describe('connection configuration', () => { it('configures busy_timeout and WAL after the key and before the first schema read', async () => { await setItem('s', 'a', 'x'); diff --git a/apps/mobile/src/lib/persist/encrypted-kv.ts b/apps/mobile/src/lib/persist/encrypted-kv.ts index 1f3adb1224..43f55fef51 100644 --- a/apps/mobile/src/lib/persist/encrypted-kv.ts +++ b/apps/mobile/src/lib/persist/encrypted-kv.ts @@ -9,6 +9,7 @@ import * as Crypto from 'expo-crypto'; import * as SecureStore from 'expo-secure-store'; import * as SQLite from 'expo-sqlite'; import migrations from '../../../drizzle/migrations'; +import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; import { PERSIST_DB_KEY } from '@/lib/storage-keys'; import { kv } from './schema'; @@ -92,7 +93,10 @@ async function generateHexKey(): Promise { } async function readOrCreateKey(): Promise { - const existing = await SecureStore.getItemAsync(PERSIST_DB_KEY); + // A transient keychain rejection must not be read as "no key": retrying it + // keeps one rejection from failing the open, which is memoized for the rest + // of the process. + const existing = await readStoredValueWithRetry(PERSIST_DB_KEY); if (existing) { return existing; } diff --git a/apps/mobile/src/lib/persist/read-cache.test.ts b/apps/mobile/src/lib/persist/read-cache.test.ts index b385e71383..5ce838bfa9 100644 --- a/apps/mobile/src/lib/persist/read-cache.test.ts +++ b/apps/mobile/src/lib/persist/read-cache.test.ts @@ -558,6 +558,24 @@ describe('cold-start restore and takeover', () => { expect(takeOverColdStartRestore()).toBeNull(); }); + it('retries a rejected identity-hint read and still hydrates the cache', async () => { + const { scopes } = createFakeKv(); + store.set(ACTIVE_USER_ID_KEY, 'u1'); + scopes.set( + 'cache:u1:1', + new Map([['read-cache', JSON.stringify(makePersistedClient({ id: 'u1' }))]]) + ); + // The keychain read can reject transiently; the restore must retry it + // rather than abandoning the whole cold-start restore. + vi.mocked(SecureStore.getItemAsync).mockRejectedValueOnce(new Error('keychain unavailable')); + const queryClient = new QueryClient(); + + await restorePersistedCacheOnColdStart(queryClient); + + expect(queryClient.getQueryData(GET_ME_QUERY_KEY)).toEqual({ id: 'u1' }); + expect(takeOverColdStartRestore()).toBe('cache:u1:1'); + }); + it('drops an expired blob instead of hydrating it', async () => { const { kv, scopes } = createFakeKv(); store.set(ACTIVE_USER_ID_KEY, 'u1'); diff --git a/apps/mobile/src/lib/persist/read-cache.ts b/apps/mobile/src/lib/persist/read-cache.ts index 03b6358c6a..ae11de9522 100644 --- a/apps/mobile/src/lib/persist/read-cache.ts +++ b/apps/mobile/src/lib/persist/read-cache.ts @@ -1,4 +1,3 @@ -import * as SecureStore from 'expo-secure-store'; import { type Query, type QueryClient } from '@tanstack/react-query'; import { createAsyncStoragePersister } from '@tanstack/query-async-storage-persister'; import { @@ -10,6 +9,7 @@ import { z } from 'zod'; import { buildAgentSessionListInput } from '@/lib/agent-session-input'; import { currentAuthEpoch, isCurrentAuthEpoch } from '@/lib/auth/auth-epoch'; +import { readStoredValueWithRetry } from '@/lib/auth/secure-store-read'; import { isSignOutActive, setSignOutActive } from '@/lib/auth/sign-out-state'; import * as encryptedKv from '@/lib/persist/encrypted-kv'; import { ACTIVE_USER_ID_KEY } from '@/lib/storage-keys'; @@ -271,13 +271,17 @@ export async function restorePersistedCacheOnColdStart(queryClient: QueryClient) // this restore immediately after scheduling it. coldStartGeneration += 1; const generation = coldStartGeneration; - // Capture the epoch before the first SecureStore read: a sign-in or sign-out + // Capture the epoch before the first stored-value read: a sign-in or sign-out // that lands while the hint read or the KV read is in flight fences the // whole restore, so it can never hydrate (or claim a scope) after the auth // epoch moved — including after a logout that cleared the query client. const epoch = currentAuthEpoch(); try { - const hintUserId = await SecureStore.getItemAsync(ACTIVE_USER_ID_KEY); + // The identity hint read is retried on a transient keychain rejection: a + // rejected read must not abandon the restore, or the profile's cached + // identity is never hydrated and the first foreground refetch has nothing + // to render. + const hintUserId = await readStoredValueWithRetry(ACTIVE_USER_ID_KEY); if (generation !== coldStartGeneration || !hintUserId || !isCurrentAuthEpoch(epoch)) { return; } diff --git a/apps/mobile/src/lib/trpc.test.ts b/apps/mobile/src/lib/trpc.test.ts index 9f821a4cc7..88c11145b5 100644 --- a/apps/mobile/src/lib/trpc.test.ts +++ b/apps/mobile/src/lib/trpc.test.ts @@ -67,12 +67,6 @@ vi.mock('expo-secure-store', () => ({ WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'WHEN_UNLOCKED_THIS_DEVICE_ONLY', })); -vi.mock('@/lib/config', () => ({ - API_BASE_URL: 'https://api.example.com', - E2E_LATENCY_MESSAGES_MS: 0, - E2E_LATENCY_SESSION_MS: 0, -})); - vi.mock('@/lib/storage-keys', () => ({ AUTH_TOKEN_KEY: 'auth-token', TOKEN_EXPIRES_AT_KEY: 'token-expires-at', diff --git a/apps/mobile/vitest.mounted.config.ts b/apps/mobile/vitest.mounted.config.ts index 46b660c661..351d4629fa 100644 --- a/apps/mobile/vitest.mounted.config.ts +++ b/apps/mobile/vitest.mounted.config.ts @@ -18,6 +18,9 @@ export default defineProject({ test: { name: 'mobile-mounted', environment: 'node', + // The app build's config module cannot load in this project; the setup + // file stubs the exports its importers read. + setupFiles: ['./vitest.setup.ts'], include: ['src/**/*.mounted.test.tsx'], // Project configs do not inherit the root test options, and this suite // runs both projects in parallel: on a loaded host (dev stack, simulator, diff --git a/apps/mobile/vitest.pure.config.ts b/apps/mobile/vitest.pure.config.ts index e1456e3fb4..c5c7676302 100644 --- a/apps/mobile/vitest.pure.config.ts +++ b/apps/mobile/vitest.pure.config.ts @@ -17,6 +17,9 @@ export default defineProject({ test: { name: 'mobile-pure', environment: 'node', + // The app build's config module cannot load in this project; the setup + // file stubs the exports its importers read. + setupFiles: ['./vitest.setup.ts'], // Project configs do not inherit the root test options, and this suite // runs both projects in parallel: on a loaded host (dev stack, simulator, // Appium) workers starve and real-timer tests exceed the 5s default. One diff --git a/apps/mobile/vitest.setup.ts b/apps/mobile/vitest.setup.ts new file mode 100644 index 0000000000..2afe3c785a --- /dev/null +++ b/apps/mobile/vitest.setup.ts @@ -0,0 +1,11 @@ +import { vi } from 'vitest'; + +// The mobile vitest projects run with no app build, so `@/lib/config` cannot +// load: its real module needs the baked `extra`. Tests that exercise modules +// importing it (the auth retry helpers and everything built on them) would +// otherwise fail before their first assertion. +// +// A Proxy answers every export Vitest resolves, and an unset value keeps the +// build-gated E2E windows closed. A test that needs a config value still mocks +// this module itself; its later registration wins over this one. +vi.mock('@/lib/config', () => new Proxy({}, { get: () => undefined, has: () => true }));