diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index d613bff4575..613c767c1ca 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -20,7 +20,8 @@ import { PanelLeftClose } from 'lucide-react-native' import type { RpcClient } from '../../../src/transport/rpc-client' -import { loadHosts, updateLastConnected, removeHost } from '../../../src/transport/host-store' +import { loadHosts, updateLastConnected } from '../../../src/transport/host-store' +import { removeHostAndCloseClient } from '../../../src/transport/host-removal-lifecycle' import { useHostClient, useCloseHost, @@ -684,13 +685,13 @@ export function HostScreen({ if (!hostId) { return } - // Why: close the shared client first so its WebSocket is gone before - // the host record disappears; otherwise the next loadHosts() the - // provider does (e.g. on remount) wouldn't find this host but the - // socket would still be open, leaking state. - closeHostClient(hostId) - await removeHost(hostId) - leaveHost() + try { + await removeHostAndCloseClient(hostId, closeHostClient) + leaveHost() + } catch { + // Why: metadata commit can fail while the host is still paired; keep the + // screen mounted so the user can retry instead of navigating away empty. + } }, [hostId, leaveHost, closeHostClient]) const navigateFromHostList = useCallback( diff --git a/mobile/app/index.tsx b/mobile/app/index.tsx index eaee0e5f935..c32a9972ad0 100644 --- a/mobile/app/index.tsx +++ b/mobile/app/index.tsx @@ -25,7 +25,8 @@ import { UsageBar } from '../src/components/AccountUsage' import AsyncStorage from '@react-native-async-storage/async-storage' -import { loadHosts, removeHost, renameHost } from '../src/transport/host-store' +import { loadHosts, renameHost } from '../src/transport/host-store' +import { removeHostAndCloseClient } from '../src/transport/host-removal-lifecycle' import { pickResumeWorktree } from '../src/worktree/resume-worktree' import type { RpcClient } from '../src/transport/rpc-client' import { @@ -711,10 +712,7 @@ export default function HomeScreen() { return } try { - // Why: close the shared client first so the WebSocket is gone - // before the host record disappears from loadHosts(). - closeHostClient(confirmRemove.id) - await removeHost(confirmRemove.id) + await removeHostAndCloseClient(confirmRemove.id, closeHostClient) setConfirmRemove(null) setHosts(await loadHosts()) } catch { diff --git a/mobile/app/settings.tsx b/mobile/app/settings.tsx index d12b13a15e8..6c936a1b01b 100644 --- a/mobile/app/settings.tsx +++ b/mobile/app/settings.tsx @@ -1,6 +1,15 @@ -import { View, Text, StyleSheet, Pressable, Linking } from 'react-native' +import { useCallback, useRef, useState } from 'react' +import { + View, + Text, + StyleSheet, + Pressable, + Linking, + ActivityIndicator, + ScrollView +} from 'react-native' import { useSafeAreaInsets } from 'react-native-safe-area-context' -import { useRouter } from 'expo-router' +import { useFocusEffect, useRouter } from 'expo-router' import { ChevronLeft, ChevronRight, @@ -11,13 +20,69 @@ import { LifeBuoy, Mic, Globe, - Terminal as TerminalIcon + Terminal as TerminalIcon, + KeyRound } from 'lucide-react-native' -import { colors, spacing, typography } from '../src/theme/mobile-theme' +import { colors, radii, spacing, typography } from '../src/theme/mobile-theme' +import { + loadPendingHostCredentialCleanupIds, + subscribePendingHostCredentialCleanup +} from '../src/transport/host-credential-cleanup' +import { retryPendingHostCredentialCleanup } from '../src/transport/host-store' export default function SettingsScreen() { const router = useRouter() const insets = useSafeAreaInsets() + const [pendingCredentialIds, setPendingCredentialIds] = useState([]) + const [retryingCredentialCleanup, setRetryingCredentialCleanup] = useState(false) + const [credentialRetryFailed, setCredentialRetryFailed] = useState(false) + const credentialRefreshGenerationRef = useRef(0) + + useFocusEffect( + useCallback(() => { + let active = true + setCredentialRetryFailed(false) + const refresh = () => { + const generation = ++credentialRefreshGenerationRef.current + void loadPendingHostCredentialCleanupIds().then((ids) => { + if (active && generation === credentialRefreshGenerationRef.current) { + setPendingCredentialIds(ids) + // Why: neutral copy once the queue is empty so a later pending + // set does not inherit a previous Retry failure message. + if (ids.length === 0) { + setCredentialRetryFailed(false) + } + } + }) + } + const unsubscribe = subscribePendingHostCredentialCleanup(refresh) + refresh() + return () => { + active = false + credentialRefreshGenerationRef.current += 1 + unsubscribe() + } + }, []) + ) + + const retryCredentialCleanup = useCallback(async () => { + if (retryingCredentialCleanup) { + return + } + setCredentialRetryFailed(false) + setRetryingCredentialCleanup(true) + try { + const result = await retryPendingHostCredentialCleanup() + setPendingCredentialIds(result.remainingIds) + setCredentialRetryFailed(result.remainingIds.length > 0) + } catch { + setCredentialRetryFailed(true) + } finally { + setRetryingCredentialCleanup(false) + } + }, [retryingCredentialCleanup]) + + const pendingCredentialCount = pendingCredentialIds.length return ( @@ -28,79 +93,121 @@ export default function SettingsScreen() { Settings - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/terminal-settings')} - > - - Terminal - - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/browser-settings')} - > - - Browser - - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/voice-settings')} - > - - Voice - - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/notifications')} - > - - Notifications - - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/troubleshoot')} - > - - Troubleshooting - - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => router.push('/about')} - > - - About - - - + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/terminal-settings')} + > + + Terminal + + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/browser-settings')} + > + + Browser + + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/voice-settings')} + > + + Voice + + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/notifications')} + > + + Notifications + + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/troubleshoot')} + > + + Troubleshooting + + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => router.push('/about')} + > + + About + + + - - [styles.row, pressed && styles.rowPressed]} - onPress={() => void Linking.openURL('https://www.onorca.dev/privacy')} - > - - Privacy Policy - - - [styles.row, pressed && styles.rowPressed]} - onPress={() => void Linking.openURL('https://github.com/stablyai/orca/issues')} - > - - Support - - + {pendingCredentialCount > 0 ? ( + + + + + Pairing credential cleanup + + {credentialRetryFailed + ? "Cleanup still couldn't be confirmed. Try again later." + : `Couldn't confirm cleanup for ${pendingCredentialCount} credential${pendingCredentialCount === 1 ? '' : 's'} on this device.`} + + + [ + styles.retryButton, + pressed && !retryingCredentialCleanup && styles.rowPressed + ]} + onPress={() => void retryCredentialCleanup()} + > + {retryingCredentialCleanup ? ( + + ) : ( + Retry + )} + + + + ) : null} + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => void Linking.openURL('https://www.onorca.dev/privacy')} + > + + Privacy Policy + + + [styles.row, pressed && styles.rowPressed]} + onPress={() => void Linking.openURL('https://github.com/stablyai/orca/issues')} + > + + Support + + + ) } @@ -109,7 +216,7 @@ const styles = StyleSheet.create({ container: { flex: 1, backgroundColor: colors.bgBase, - padding: spacing.lg + paddingHorizontal: spacing.lg }, topRow: { flexDirection: 'row', @@ -153,6 +260,40 @@ const styles = StyleSheet.create({ fontWeight: '500', color: colors.textPrimary }, + credentialCleanupRow: { + flexDirection: 'row', + alignItems: 'center', + gap: spacing.sm + 2, + paddingVertical: spacing.md, + paddingHorizontal: spacing.md + 2 + }, + credentialCleanupCopy: { + flex: 1, + gap: spacing.xs + }, + credentialCleanupTitle: { + fontSize: typography.bodySize, + fontWeight: '500', + color: colors.textPrimary + }, + rowHint: { + fontSize: typography.metaSize, + color: colors.textSecondary, + lineHeight: 17 + }, + retryButton: { + width: 72, + height: 32, + borderRadius: radii.button, + backgroundColor: colors.bgRaised, + alignItems: 'center', + justifyContent: 'center' + }, + retryButtonText: { + fontSize: typography.metaSize, + fontWeight: '600', + color: colors.textPrimary + }, separator: { height: StyleSheet.hairlineWidth, backgroundColor: colors.borderSubtle, diff --git a/mobile/src/transport/client-context.test.ts b/mobile/src/transport/client-context.test.ts index e9a298b6a74..b2ed60e74e4 100644 --- a/mobile/src/transport/client-context.test.ts +++ b/mobile/src/transport/client-context.test.ts @@ -119,6 +119,7 @@ async function renderHarness(hostId: string): Promise { } beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true connectMock.mockReset() loadHostsMock.mockReset() }) @@ -156,4 +157,76 @@ describe('useHostClient', () => { harness.unmount() }) + + it('does not open a client after the host is closed during an in-flight lookup', async () => { + let resolveHosts: ((hosts: (typeof HOST)[]) => void) | null = null + const hostLookup = new Promise<(typeof HOST)[]>((resolve) => { + resolveHosts = resolve + }) + const fake = makeFakeClient('connected') + connectMock.mockReturnValue(fake) + loadHostsMock.mockReturnValue(hostLookup) + + let closeHost: ((hostId: string) => void) | null = null + let renderer: ReactTestRenderer | null = null + function Probe(): null { + closeHost = useCloseHost() + useHostClient(HOST.id) + return null + } + + const restore = suppressReactTestRendererDeprecationWarning() + try { + act(() => { + renderer = create(createElement(RpcClientProvider, null, createElement(Probe))) + }) + } finally { + restore() + } + expect(loadHostsMock).toHaveBeenCalledOnce() + if (!closeHost || !resolveHosts || !renderer) { + throw new Error('pending-open harness did not initialize') + } + + act(() => closeHost?.(HOST.id)) + await act(async () => { + resolveHosts?.([HOST]) + await hostLookup + }) + + expect(connectMock).not.toHaveBeenCalled() + expect(fake.closeMock).not.toHaveBeenCalled() + act(() => renderer.unmount()) + }) + + it('does not open a client after provider unmount during an in-flight lookup', async () => { + let resolveHosts: ((hosts: (typeof HOST)[]) => void) | null = null + const hostLookup = new Promise<(typeof HOST)[]>((resolve) => { + resolveHosts = resolve + }) + connectMock.mockReturnValue(makeFakeClient('connected')) + loadHostsMock.mockReturnValue(hostLookup) + + let renderer: ReactTestRenderer | null = null + function Probe(): null { + useHostClient(HOST.id) + return null + } + const restore = suppressReactTestRendererDeprecationWarning() + try { + act(() => { + renderer = create(createElement(RpcClientProvider, null, createElement(Probe))) + }) + } finally { + restore() + } + expect(loadHostsMock).toHaveBeenCalledOnce() + act(() => renderer?.unmount()) + await act(async () => { + resolveHosts?.([HOST]) + await hostLookup + }) + + expect(connectMock).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/transport/client-context.tsx b/mobile/src/transport/client-context.tsx index 41f1b5092e8..df54f143528 100644 --- a/mobile/src/transport/client-context.tsx +++ b/mobile/src/transport/client-context.tsx @@ -22,6 +22,7 @@ import { import { connect, type RpcClient } from './rpc-client' import { connectionLogStore } from './connection-log-buffer' import { subscribeConnectionRevivalTriggers } from './connection-revival-triggers' +import { HostClientOpenRegistry } from './host-client-open-registry' import { loadHosts } from './host-store' import type { ConnectionState, HostProfile } from './types' @@ -66,7 +67,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { // Pending opens (avoid two acquire() callers in the same render racing the // host lookup). Keyed by hostId, value is a sentinel resolved when the // entry materialises. - const pendingOpensRef = useRef>>(new Map()) + const pendingOpensRef = useRef(new HostClientOpenRegistry()) // Why: a fast-path cache of already-loaded HostProfiles. Screens that // have run loadHosts() can call primeHosts() to populate this and skip @@ -91,19 +92,18 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { } const closeEntry = useCallback((hostId: string) => { + pendingOpensRef.current.cancel(hostId) + primedHostsRef.current.delete(hostId) const entry = storeRef.current.get(hostId) - if (!entry) { - return - } - entry.unsubState() - entry.client.close() + entry?.unsubState() storeRef.current.delete(hostId) + entry?.client.close() notifyHostState(hostId, 'disconnected') notifyAllHosts() }, []) const openEntry = useCallback(async (hostId: string): Promise => { - const existing = pendingOpensRef.current.get(hostId) + const existing = pendingOpensRef.current.getActivePromise(hostId) if (existing) { await existing return storeRef.current.get(hostId) ?? null @@ -112,7 +112,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { const promise = new Promise((res) => { resolve = res }) - pendingOpensRef.current.set(hostId, promise) + const pendingOpen = pendingOpensRef.current.register(hostId, promise) try { // Why: prefer the primed cache (populated by primeHosts when the @@ -143,6 +143,10 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { } } + if (pendingOpen.cancelled) { + return null + } + // Re-check after any await — another acquire() may have completed. const after = storeRef.current.get(hostId) if (after) { @@ -184,7 +188,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { notifyAllHosts() return entry } finally { - pendingOpensRef.current.delete(hostId) + pendingOpensRef.current.deleteIfCurrent(hostId, pendingOpen) resolve() } }, []) @@ -326,6 +330,7 @@ export function RpcClientProvider({ children }: { children: ReactNode }) { useEffect(() => { const store = storeRef.current return () => { + pendingOpensRef.current.cancelAll() for (const [hostId] of store) { closeEntry(hostId) } diff --git a/mobile/src/transport/host-client-open-registry.ts b/mobile/src/transport/host-client-open-registry.ts new file mode 100644 index 00000000000..a47ca5a231a --- /dev/null +++ b/mobile/src/transport/host-client-open-registry.ts @@ -0,0 +1,42 @@ +export type HostClientOpenTicket = { + cancelled: boolean + promise: Promise +} + +export class HostClientOpenRegistry { + private readonly pending = new Map() + + getActivePromise(hostId: string): Promise | null { + const ticket = this.pending.get(hostId) + return ticket && !ticket.cancelled ? ticket.promise : null + } + + register(hostId: string, promise: Promise): HostClientOpenTicket { + const ticket = { cancelled: false, promise } + this.pending.set(hostId, ticket) + return ticket + } + + cancel(hostId: string): void { + const ticket = this.pending.get(hostId) + if (ticket) { + ticket.cancelled = true + // Why: the host lookup may never settle; release the registry's strong + // reference immediately while the ticket still cancels its continuation. + this.pending.delete(hostId) + } + } + + deleteIfCurrent(hostId: string, ticket: HostClientOpenTicket): void { + if (this.pending.get(hostId) === ticket) { + this.pending.delete(hostId) + } + } + + cancelAll(): void { + for (const ticket of this.pending.values()) { + ticket.cancelled = true + } + this.pending.clear() + } +} diff --git a/mobile/src/transport/host-credential-cleanup.test.ts b/mobile/src/transport/host-credential-cleanup.test.ts new file mode 100644 index 00000000000..44e2371768e --- /dev/null +++ b/mobile/src/transport/host-credential-cleanup.test.ts @@ -0,0 +1,227 @@ +import AsyncStorage from '@react-native-async-storage/async-storage' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + loadPendingHostCredentialCleanupIds, + resetHostCredentialCleanupForTests, + retryPendingHostCredentialCleanups, + scheduleHostCredentialCleanup, + subscribePendingHostCredentialCleanup +} from './host-credential-cleanup' + +const asyncStorageMock = vi.hoisted(() => ({ + getItem: vi.fn(), + setItem: vi.fn() +})) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: asyncStorageMock +})) + +const PENDING_STORAGE_KEY = 'orca:pending-host-credential-cleanups' +let storedPendingIds: string[] +let readShouldFail = false + +async function flushMicrotasks(): Promise { + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() +} + +describe('host credential cleanup', () => { + beforeEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + resetHostCredentialCleanupForTests() + storedPendingIds = [] + readShouldFail = false + asyncStorageMock.getItem.mockImplementation(async (key: string) => { + if (key !== PENDING_STORAGE_KEY) { + return null + } + if (readShouldFail) { + throw new Error('async storage unavailable') + } + return JSON.stringify(storedPendingIds) + }) + asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { + if (key === PENDING_STORAGE_KEY) { + storedPendingIds = JSON.parse(raw) as string[] + } + }) + }) + + afterEach(() => { + vi.useRealTimers() + resetHostCredentialCleanupForTests() + }) + + it('clears the durable intent after a successful cleanup', async () => { + const deleteCredential = vi.fn().mockResolvedValue(undefined) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 20) + await vi.waitFor(async () => { + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + expect(deleteCredential).toHaveBeenCalledOnce() + }) + + it('persists cleanup intent before a rejected delete for manual retry', async () => { + const deleteCredential = vi.fn().mockRejectedValue(new Error('keychain unavailable')) + const listener = vi.fn() + const unsubscribe = subscribePendingHostCredentialCleanup(listener) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 20) + await vi.waitFor(() => expect(deleteCredential).toHaveBeenCalledOnce()) + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-1']) + expect(listener).toHaveBeenCalled() + unsubscribe() + }) + + it('records intent before a stalled delete times out', async () => { + vi.useFakeTimers() + const deleteCredential = vi.fn(() => new Promise(() => undefined)) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 3_000) + await flushMicrotasks() + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-1']) + + await vi.advanceTimersByTimeAsync(3_000) + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-1']) + expect(deleteCredential).toHaveBeenCalledOnce() + }) + + it('returns after durable intent without waiting for the native delete', async () => { + vi.useFakeTimers() + const deleteCredential = vi.fn(() => new Promise(() => undefined)) + + await expect( + scheduleHostCredentialCleanup('host-1', deleteCredential, 3_000) + ).resolves.toBeUndefined() + await flushMicrotasks() + + expect(storedPendingIds).toEqual(['host-1']) + expect(deleteCredential).toHaveBeenCalledOnce() + + await vi.advanceTimersByTimeAsync(3_000) + }) + + it('clears a timed-out pending entry when the native delete later succeeds', async () => { + vi.useFakeTimers() + let resolveDelete: (() => void) | null = null + const deleteCredential = vi.fn( + () => + new Promise((resolve) => { + resolveDelete = resolve + }) + ) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 3_000) + await flushMicrotasks() + expect(deleteCredential).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(3_000) + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-1']) + expect(resolveDelete).not.toBeNull() + + resolveDelete?.() + await vi.waitFor(async () => { + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + }) + + it('joins a concurrently observed delete instead of stacking native calls', async () => { + let resolveDelete: (() => void) | null = null + const deleteCredential = vi.fn( + () => + new Promise((resolve) => { + resolveDelete = resolve + }) + ) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 1_000) + await vi.waitFor(() => expect(deleteCredential).toHaveBeenCalledOnce()) + const readsBeforeSecondAttempt = asyncStorageMock.getItem.mock.calls.length + await scheduleHostCredentialCleanup('host-1', deleteCredential, 1_000) + await vi.waitFor(() => { + expect(asyncStorageMock.getItem.mock.calls.length).toBeGreaterThan(readsBeforeSecondAttempt) + }) + await flushMicrotasks() + expect(deleteCredential).toHaveBeenCalledOnce() + + resolveDelete?.() + await vi.waitFor(async () => { + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + }) + + it('starts a fresh native attempt when the user retries after a timeout', async () => { + vi.useFakeTimers() + const deleteCredential = vi + .fn() + .mockImplementationOnce(() => new Promise(() => undefined)) + .mockResolvedValueOnce(undefined) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 3_000) + await flushMicrotasks() + expect(deleteCredential).toHaveBeenCalledOnce() + await vi.advanceTimersByTimeAsync(3_000) + + await expect(retryPendingHostCredentialCleanups(deleteCredential)).resolves.toEqual({ + clearedCount: 1, + remainingIds: [] + }) + expect(deleteCredential).toHaveBeenCalledTimes(2) + }) + + it('retries only on an explicit call and clears successful ids', async () => { + storedPendingIds = ['host-1', 'host-2'] + const deleteCredential = vi + .fn() + .mockRejectedValueOnce(new Error('still unavailable')) + .mockResolvedValue(undefined) + + expect(deleteCredential).not.toHaveBeenCalled() + await expect(retryPendingHostCredentialCleanups(deleteCredential)).resolves.toEqual({ + clearedCount: 1, + remainingIds: ['host-1'] + }) + expect(deleteCredential).toHaveBeenCalledTimes(2) + }) + + it('does not wipe existing pending ids when storage read fails during mutation', async () => { + storedPendingIds = ['host-a'] + const deleteCredential = vi.fn().mockRejectedValue(new Error('keychain unavailable')) + + readShouldFail = true + await scheduleHostCredentialCleanup('host-b', deleteCredential, 20) + await vi.waitFor(() => expect(deleteCredential).toHaveBeenCalledOnce()) + readShouldFail = false + + // Native delete still runs; durable queue is left untouched (not clobbered). + expect(deleteCredential).toHaveBeenCalledOnce() + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-a']) + expect(asyncStorageMock.setItem).not.toHaveBeenCalled() + }) + + it('cleans a new credential when the same host id is paired and removed again', async () => { + const deleteCredential = vi.fn().mockResolvedValue(undefined) + + await scheduleHostCredentialCleanup('host-1', deleteCredential, 20) + await vi.waitFor(async () => { + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + await scheduleHostCredentialCleanup('host-1', deleteCredential, 20) + await vi.waitFor(async () => { + expect(deleteCredential).toHaveBeenCalledTimes(2) + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + + expect(deleteCredential).toHaveBeenCalledTimes(2) + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) + + it('soft-loads empty when pending-cleanup storage is malformed', async () => { + vi.mocked(AsyncStorage.getItem).mockResolvedValueOnce('{') + + await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual([]) + }) +}) diff --git a/mobile/src/transport/host-credential-cleanup.ts b/mobile/src/transport/host-credential-cleanup.ts new file mode 100644 index 00000000000..694a463a3c9 --- /dev/null +++ b/mobile/src/transport/host-credential-cleanup.ts @@ -0,0 +1,199 @@ +import AsyncStorage from '@react-native-async-storage/async-storage' + +const PENDING_STORAGE_KEY = 'orca:pending-host-credential-cleanups' +const CLEANUP_CONFIRM_TIMEOUT_MS = 3_000 + +type DeleteHostCredential = (hostId: string) => Promise +type CleanupAttemptResult = 'cleared' | 'pending' +type CleanupOutcome = 'cleared' | 'failed' | 'timed-out' +type PendingIdsRead = { ok: true; ids: string[] } | { ok: false } + +let pendingMutation: Promise = Promise.resolve() +const pendingListeners = new Set<() => void>() +// Why: concurrent taps/callers share one native operation while it is being +// confirmed. A timed-out operation is released so the next user tap can retry. +const inflightDeletes = new Map>() + +function parsePendingIds(raw: string): string[] | null { + try { + const parsed = JSON.parse(raw) as unknown + if (!Array.isArray(parsed)) { + return null + } + return [...new Set(parsed.filter((value): value is string => typeof value === 'string'))] + } catch { + return null + } +} + +function sameIdList(a: string[], b: string[]): boolean { + return a.length === b.length && a.every((id, index) => id === b[index]) +} + +async function readPendingIdsForMutation(): Promise { + try { + const raw = await AsyncStorage.getItem(PENDING_STORAGE_KEY) + if (raw === null) { + return { ok: true, ids: [] } + } + const ids = parsePendingIds(raw) + if (!ids) { + // Why: refuse to RMW over unreadable payload — treating it as [] would + // wipe durable pending ids on the next add/remove. + return { ok: false } + } + return { ok: true, ids } + } catch { + return { ok: false } + } +} + +async function readPendingIdsSoft(): Promise { + const result = await readPendingIdsForMutation() + return result.ok ? result.ids : [] +} + +async function mutatePendingIds(update: (ids: string[]) => string[]): Promise { + const mutation = pendingMutation.then(async () => { + const current = await readPendingIdsForMutation() + if (!current.ok) { + throw new Error('pending host credential cleanup storage unreadable') + } + const next = update(current.ids) + if (sameIdList(current.ids, next)) { + return + } + await AsyncStorage.setItem(PENDING_STORAGE_KEY, JSON.stringify(next)) + for (const listener of pendingListeners) { + listener() + } + }) + pendingMutation = mutation.catch(() => {}) + return mutation +} + +async function addPendingId(hostId: string): Promise { + await mutatePendingIds((ids) => (ids.includes(hostId) ? ids : [...ids, hostId])) +} + +async function removePendingId(hostId: string): Promise { + await mutatePendingIds((ids) => ids.filter((id) => id !== hostId)) +} + +function observeCleanup(cleanup: Promise, timeoutMs: number): Promise { + return new Promise((resolve) => { + let settled = false + const finish = (outcome: CleanupOutcome) => { + if (settled) { + return + } + settled = true + clearTimeout(timeout) + resolve(outcome) + } + const timeout = setTimeout(() => finish('timed-out'), timeoutMs) + cleanup.then( + () => finish('cleared'), + () => finish('failed') + ) + }) +} + +function startOrJoinDelete(hostId: string, deleteCredential: DeleteHostCredential): Promise { + const existing = inflightDeletes.get(hostId) + if (existing) { + return existing + } + const cleanup = Promise.resolve() + .then(() => deleteCredential(hostId)) + .finally(() => { + if (inflightDeletes.get(hostId) === cleanup) { + inflightDeletes.delete(hostId) + } + }) + inflightDeletes.set(hostId, cleanup) + return cleanup +} + +async function recordCleanupIntent(hostId: string): Promise { + try { + await addPendingId(hostId) + } catch { + // Best-effort native delete can still proceed without a durable row. + } +} + +async function confirmNativeCleanup( + hostId: string, + deleteCredential: DeleteHostCredential, + timeoutMs: number +): Promise { + const cleanup = startOrJoinDelete(hostId, deleteCredential) + // Why: attach before observing so a success that races the confirm timeout + // still clears the durable queue entry (including after timed-out returns). + const clearWhenDeleted = cleanup.then( + () => removePendingId(hostId).catch(() => undefined), + () => undefined + ) + const outcome = await observeCleanup(cleanup, timeoutMs) + if (outcome === 'cleared') { + await clearWhenDeleted + return 'cleared' + } + + if (outcome === 'timed-out' && inflightDeletes.get(hostId) === cleanup) { + // Why: timing out is the boundary between automatic work and a future + // user-owned retry. Releasing only the dedupe entry does not start work. + inflightDeletes.delete(hostId) + } + + // Why: failed/unconfirmed attempts stay user-owned; nothing auto-retries. + void clearWhenDeleted + return 'pending' +} + +export async function loadPendingHostCredentialCleanupIds(): Promise { + await pendingMutation + return readPendingIdsSoft() +} + +export function subscribePendingHostCredentialCleanup(listener: () => void): () => void { + pendingListeners.add(listener) + return () => pendingListeners.delete(listener) +} + +/** + * Await only durable intent (AsyncStorage). Native keychain delete is + * fire-and-forget so removeHost never blocks on SecureStore. + */ +export async function scheduleHostCredentialCleanup( + hostId: string, + deleteCredential: DeleteHostCredential, + timeoutMs = CLEANUP_CONFIRM_TIMEOUT_MS +): Promise { + await recordCleanupIntent(hostId) + void confirmNativeCleanup(hostId, deleteCredential, timeoutMs).catch(() => {}) +} + +export async function retryPendingHostCredentialCleanups( + deleteCredential: DeleteHostCredential +): Promise<{ clearedCount: number; remainingIds: string[] }> { + const ids = await loadPendingHostCredentialCleanupIds() + const outcomes = await Promise.all( + // Why: these ids are already durable. Re-adding intent can race a late + // success and recreate a ghost row after the credential was deleted. + ids.map((id) => confirmNativeCleanup(id, deleteCredential, CLEANUP_CONFIRM_TIMEOUT_MS)) + ) + const remainingIds = await loadPendingHostCredentialCleanupIds() + return { + clearedCount: outcomes.filter((outcome) => outcome === 'cleared').length, + remainingIds + } +} + +/** Test-only: drop module listeners/in-flight state between cases. */ +export function resetHostCredentialCleanupForTests(): void { + inflightDeletes.clear() + pendingListeners.clear() + pendingMutation = Promise.resolve() +} diff --git a/mobile/src/transport/host-removal-lifecycle.test.ts b/mobile/src/transport/host-removal-lifecycle.test.ts new file mode 100644 index 00000000000..e398062b270 --- /dev/null +++ b/mobile/src/transport/host-removal-lifecycle.test.ts @@ -0,0 +1,42 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const removeHostMock = vi.hoisted(() => vi.fn()) + +vi.mock('./host-store', () => ({ + removeHost: (hostId: string) => removeHostMock(hostId) +})) + +import { removeHostAndCloseClient } from './host-removal-lifecycle' + +describe('host removal lifecycle', () => { + beforeEach(() => { + removeHostMock.mockReset() + }) + + it('closes the client only after metadata removal commits', async () => { + let commitRemoval: (() => void) | null = null + removeHostMock.mockReturnValue( + new Promise((resolve) => { + commitRemoval = resolve + }) + ) + const closeHostClient = vi.fn() + + const removal = removeHostAndCloseClient('host-1', closeHostClient) + expect(closeHostClient).not.toHaveBeenCalled() + commitRemoval?.() + await removal + + expect(closeHostClient).toHaveBeenCalledWith('host-1') + }) + + it('keeps the client open when metadata removal fails', async () => { + removeHostMock.mockRejectedValue(new Error('storage unavailable')) + const closeHostClient = vi.fn() + + await expect(removeHostAndCloseClient('host-1', closeHostClient)).rejects.toThrow( + 'storage unavailable' + ) + expect(closeHostClient).not.toHaveBeenCalled() + }) +}) diff --git a/mobile/src/transport/host-removal-lifecycle.ts b/mobile/src/transport/host-removal-lifecycle.ts new file mode 100644 index 00000000000..ba599614804 --- /dev/null +++ b/mobile/src/transport/host-removal-lifecycle.ts @@ -0,0 +1,11 @@ +import { removeHost } from './host-store' + +export async function removeHostAndCloseClient( + hostId: string, + closeHostClient: (hostId: string) => void +): Promise { + // Why: closing before the metadata commit can strand a still-paired host on + // storage failure; closing immediately after success prevents socket leaks. + await removeHost(hostId) + closeHostClient(hostId) +} diff --git a/mobile/src/transport/host-store.test.ts b/mobile/src/transport/host-store.test.ts new file mode 100644 index 00000000000..7c879c96f81 --- /dev/null +++ b/mobile/src/transport/host-store.test.ts @@ -0,0 +1,167 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const asyncStorageMock = vi.hoisted(() => ({ + getItem: vi.fn(), + setItem: vi.fn(), + removeItem: vi.fn() +})) + +const secureStoreMock = vi.hoisted(() => ({ + deleteItemAsync: vi.fn(), + getItemAsync: vi.fn(), + setItemAsync: vi.fn() +})) + +const scheduleCleanupMock = vi.hoisted(() => vi.fn()) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: asyncStorageMock +})) + +vi.mock('expo-secure-store', () => ({ + WHEN_UNLOCKED_THIS_DEVICE_ONLY: 'WHEN_UNLOCKED_THIS_DEVICE_ONLY', + ...secureStoreMock +})) + +vi.mock('react-native', () => ({ + Platform: { OS: 'ios' } +})) + +vi.mock('./host-credential-cleanup', () => ({ + scheduleHostCredentialCleanup: (...args: unknown[]) => scheduleCleanupMock(...args), + retryPendingHostCredentialCleanups: vi.fn() +})) + +import { removeHost, renameHost, resetHostStoreForTests, updateLastConnected } from './host-store' + +const HOSTS_STORAGE_KEY = 'orca:hosts' +const HOST_ONE = { + id: 'host-1', + name: 'Host 1', + endpoint: 'ws://127.0.0.1:1', + publicKeyB64: 'key', + lastConnected: 0 +} +const HOST_TWO = { + id: 'host-2', + name: 'Host 2', + endpoint: 'ws://127.0.0.1:2', + publicKeyB64: 'key-2', + lastConnected: 0 +} + +describe('host-store list mutations', () => { + let storedHostsRaw: string + + beforeEach(() => { + vi.clearAllMocks() + resetHostStoreForTests() + scheduleCleanupMock.mockReset() + scheduleCleanupMock.mockResolvedValue(undefined) + storedHostsRaw = JSON.stringify([HOST_ONE, HOST_TWO]) + asyncStorageMock.getItem.mockImplementation(async (key: string) => { + if (key === HOSTS_STORAGE_KEY) { + return storedHostsRaw + } + return null + }) + asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { + if (key === HOSTS_STORAGE_KEY) { + storedHostsRaw = raw + } + }) + }) + + it('commits the removal when credential cleanup scheduling rejects', async () => { + scheduleCleanupMock.mockRejectedValue(new Error('intent storage unavailable')) + + await expect(removeHost(HOST_ONE.id)).resolves.toBeUndefined() + + expect(JSON.parse(storedHostsRaw)).toEqual([HOST_TWO]) + expect(scheduleCleanupMock).toHaveBeenCalledWith(HOST_ONE.id, expect.any(Function)) + }) + + it('awaits cleanup scheduling after metadata commit', async () => { + let resolveSchedule: (() => void) | null = null + scheduleCleanupMock.mockReturnValue( + new Promise((resolve) => { + resolveSchedule = resolve + }) + ) + + let settled = false + const removal = removeHost(HOST_ONE.id).then(() => { + settled = true + }) + await vi.waitFor(() => { + expect(JSON.parse(storedHostsRaw)).toEqual([HOST_TWO]) + }) + expect(settled).toBe(false) + expect(scheduleCleanupMock).toHaveBeenCalledOnce() + + resolveSchedule?.() + await removal + expect(settled).toBe(true) + }) + + it('applies concurrent rename and remove without clobbering either', async () => { + let releaseReads: (() => void) | null = null + const readsReleased = new Promise((resolve) => { + releaseReads = resolve + }) + let pendingReads = 0 + asyncStorageMock.getItem.mockImplementation(async (key: string) => { + if (key !== HOSTS_STORAGE_KEY) { + return null + } + pendingReads += 1 + if (pendingReads <= 2) { + await readsReleased + } + return storedHostsRaw + }) + + const rename = renameHost(HOST_ONE.id, 'Renamed Host') + const remove = removeHost(HOST_TWO.id) + // Both writers have started their RMW and are blocked on the shared read + // gate; without a mutation queue the second would clobber the first. + await vi.waitFor(() => { + expect(pendingReads).toBeGreaterThanOrEqual(1) + }) + releaseReads?.() + await Promise.all([rename, remove]) + + expect(JSON.parse(storedHostsRaw)).toEqual([ + { + ...HOST_ONE, + name: 'Renamed Host' + } + ]) + }) + + it('preserves a rename when lastConnected updates race it', async () => { + const before = Date.now() + await Promise.all([ + renameHost(HOST_ONE.id, 'Alpha'), + updateLastConnected(HOST_ONE.id), + renameHost(HOST_TWO.id, 'Beta') + ]) + + const stored = JSON.parse(storedHostsRaw) as Array + expect(stored).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: HOST_ONE.id, name: 'Alpha' }), + expect.objectContaining({ id: HOST_TWO.id, name: 'Beta' }) + ]) + ) + const hostOne = stored.find((host) => host.id === HOST_ONE.id) + expect(hostOne?.lastConnected).toBeGreaterThanOrEqual(before) + }) + + it('does not wipe the host list when storage is unreadable during mutation', async () => { + storedHostsRaw = '{' + await expect(renameHost(HOST_ONE.id, 'Nope')).rejects.toThrow(/unreadable/) + expect(asyncStorageMock.setItem).not.toHaveBeenCalled() + expect(storedHostsRaw).toBe('{') + }) +}) diff --git a/mobile/src/transport/host-store.ts b/mobile/src/transport/host-store.ts index feb9eb477cb..33a0c929c6d 100644 --- a/mobile/src/transport/host-store.ts +++ b/mobile/src/transport/host-store.ts @@ -8,6 +8,10 @@ import { type StoredHostProfile } from './types' import { getNextHostNameFromHosts } from './host-names' +import { + retryPendingHostCredentialCleanups, + scheduleHostCredentialCleanup +} from './host-credential-cleanup' const STORAGE_KEY = 'orca:hosts' // Why: SecureStore keys must match [A-Za-z0-9._-]; colons are rejected. @@ -66,8 +70,39 @@ async function deleteDeviceToken(hostId: string): Promise { // (cleared on app uninstall, persisted across foreground/background). const tokenCache = new Map() let inflightLoad: Promise | null = null +// Why: rename / lastConnected / remove / save all RMW the same hosts JSON. +// Without a queue, concurrent writers re-read a stale snapshot and the last +// setItem wins — resurrecting a removed host or dropping a rename. +let hostListMutation: Promise = Promise.resolve() + +function parseStoredHosts(raw: string | null): StoredHostProfile[] | null { + if (!raw) { + return [] + } + try { + const parsed = JSON.parse(raw) as unknown + if (!Array.isArray(parsed)) { + return null + } + return parsed.flatMap((item) => { + // Why: pre-v0.0.3 records carry the deviceToken in AsyncStorage. + // Drop them silently — the three pre-launch users will re-pair on + // first run rather than carry a migration shim through the auth path. + if (item && typeof item === 'object' && 'deviceToken' in item) { + return [] + } + const result = StoredHostProfileSchema.safeParse(item) + return result.success ? [result.data] : [] + }) + } catch { + return null + } +} export async function loadHosts(): Promise { + // Why: writers hold the mutation chain across their full RMW; wait so a + // load right after rename/remove does not race a half-written list. + await hostListMutation // Why: deduplicate concurrent loadHosts() calls so multiple screens // mounting simultaneously share one Keychain read pass. if (inflightLoad) { @@ -81,38 +116,18 @@ export async function loadHosts(): Promise { async function doLoadHosts(): Promise { const raw = await AsyncStorage.getItem(STORAGE_KEY) - if (!raw) { - return [] - } - let parsed: unknown - try { - parsed = JSON.parse(raw) - } catch { - return [] - } - if (!Array.isArray(parsed)) { + const storedHosts = parseStoredHosts(raw) + if (!storedHosts) { return [] } const out: HostProfile[] = [] - for (const item of parsed) { - // Why: pre-v0.0.3 records carry the deviceToken in AsyncStorage. - // Drop them silently — the three pre-launch users will re-pair on - // first run rather than carry a migration shim through the auth - // path. - if (item && typeof item === 'object' && 'deviceToken' in item) { - continue - } - const stored = StoredHostProfileSchema.safeParse(item) - if (!stored.success) { - continue - } - - let token = tokenCache.get(stored.data.id) + for (const stored of storedHosts) { + let token = tokenCache.get(stored.id) if (!token) { let fetched: string | null try { - fetched = await readDeviceToken(stored.data.id) + fetched = await readDeviceToken(stored.id) } catch { // Why: a transient Keychain failure for one entry (e.g. // errSecInteractionNotAllowed while the device is briefly locked, @@ -127,37 +142,50 @@ async function doLoadHosts(): Promise { continue } token = fetched - tokenCache.set(stored.data.id, token) + tokenCache.set(stored.id, token) } - out.push({ ...stored.data, deviceToken: token }) + out.push({ ...stored, deviceToken: token }) } return out } async function loadStoredHosts(): Promise { - const raw = await AsyncStorage.getItem(STORAGE_KEY) - if (!raw) { - return [] - } try { - const parsed = JSON.parse(raw) as unknown - if (!Array.isArray(parsed)) { - return [] - } - return parsed.flatMap((item) => { - // Why: same drop-old-records rule as loadHosts; keeps internal - // mutators from re-persisting pre-v0.0.3 entries. - if (item && typeof item === 'object' && 'deviceToken' in item) { - return [] - } - const result = StoredHostProfileSchema.safeParse(item) - return result.success ? [result.data] : [] - }) + return parseStoredHosts(await AsyncStorage.getItem(STORAGE_KEY)) ?? [] } catch { return [] } } +async function readStoredHostsForMutation(): Promise { + try { + const parsed = parseStoredHosts(await AsyncStorage.getItem(STORAGE_KEY)) + if (!parsed) { + // Why: refuse to RMW over unreadable payload — treating it as [] would + // wipe the durable host list on the next rename/remove/save. + throw new Error('host list storage unreadable') + } + return parsed + } catch (error) { + if (error instanceof Error && error.message === 'host list storage unreadable') { + throw error + } + throw new Error('host list storage unreadable') + } +} + +async function mutateStoredHosts( + update: (hosts: StoredHostProfile[]) => StoredHostProfile[] +): Promise { + const mutation = hostListMutation.then(async () => { + const current = await readStoredHostsForMutation() + const next = update(current) + await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(next)) + }) + hostListMutation = mutation.catch(() => {}) + return mutation +} + function toStored(host: HostProfile): StoredHostProfile { return { id: host.id, @@ -170,51 +198,77 @@ function toStored(host: HostProfile): StoredHostProfile { export async function saveHost(host: HostProfile): Promise { const validated = HostProfileSchema.parse(host) - const hosts = await loadStoredHosts() const stored = toStored(validated) - const index = hosts.findIndex((h) => h.id === stored.id) - if (index >= 0) { - hosts[index] = stored - } else { - hosts.push(stored) - } + await mutateStoredHosts((hosts) => { + const index = hosts.findIndex((h) => h.id === stored.id) + if (index >= 0) { + const next = hosts.slice() + next[index] = stored + return next + } + return [...hosts, stored] + }) // Why: write metadata BEFORE the keychain token so a crash between the two // leaves orphaned metadata (which loadHosts skips and removeHost can clean // up) rather than an orphaned keychain token with no metadata pointer — // the latter would persist forever since removeHost only deletes by hostId // from current metadata. - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(hosts)) await writeDeviceToken(stored.id, validated.deviceToken) tokenCache.set(stored.id, validated.deviceToken) } export async function removeHost(hostId: string): Promise { - const hosts = await loadStoredHosts() - const filtered = hosts.filter((h) => h.id !== hostId) - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(filtered)) - await deleteDeviceToken(hostId) + await mutateStoredHosts((hosts) => hosts.filter((h) => h.id !== hostId)) tokenCache.delete(hostId) -} - -export async function renameHost(hostId: string, newName: string): Promise { - const hosts = await loadStoredHosts() - const host = hosts.find((h) => h.id === hostId) - if (host) { - host.name = newName - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(hosts)) + // Why: await only the durable cleanup intent (AsyncStorage). Native keychain + // delete can reject or stall and must not freeze removeHost / the UI. + try { + await scheduleHostCredentialCleanup(hostId, deleteDeviceToken) + } catch { + // Metadata is already committed; orphan-token recovery is best-effort. } } +export async function retryPendingHostCredentialCleanup(): Promise<{ + clearedCount: number + remainingIds: string[] +}> { + return retryPendingHostCredentialCleanups(deleteDeviceToken) +} + +export async function renameHost(hostId: string, newName: string): Promise { + await mutateStoredHosts((hosts) => { + const index = hosts.findIndex((h) => h.id === hostId) + if (index < 0) { + return hosts + } + const next = hosts.slice() + next[index] = { ...next[index]!, name: newName } + return next + }) +} + export async function getNextHostName(): Promise { + await hostListMutation const hosts = await loadStoredHosts() return getNextHostNameFromHosts(hosts) } export async function updateLastConnected(hostId: string): Promise { - const hosts = await loadStoredHosts() - const host = hosts.find((h) => h.id === hostId) - if (host) { - host.lastConnected = Date.now() - await AsyncStorage.setItem(STORAGE_KEY, JSON.stringify(hosts)) - } + await mutateStoredHosts((hosts) => { + const index = hosts.findIndex((h) => h.id === hostId) + if (index < 0) { + return hosts + } + const next = hosts.slice() + next[index] = { ...next[index]!, lastConnected: Date.now() } + return next + }) +} + +/** Test-only: drain module mutation chain between cases. */ +export function resetHostStoreForTests(): void { + hostListMutation = Promise.resolve() + tokenCache.clear() + inflightLoad = null }