xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(mobile): don't orphan pairing tokens or leak rejections on host remove (#8317) (#8354) 

Prod-release-scan P1+P2 from v1.4.137-rc.1 mobile host-remove.

P1: Host remove could orphan a SecureStore pairing token with no Settings
retry when BOTH the durable pending-queue write failed AND the native delete
rejected/stalled. recordCleanupIntent swallowed the queue-write failure, so
the only recovery handle for the failed keychain delete was silently lost.
Now scheduleHostCredentialCleanup keeps a session-scoped in-memory fallback
handle when the durable write fails, so Settings still surfaces the pending
cleanup and offers a retry; confirmNativeCleanup clears the fallback if the
native delete later lands. removeHost stays non-blocking on the keychain
(freeze fix intact).

P2 (updateLastConnected): the fire-and-forget `void updateLastConnected(...)`
call site threw on unreadable storage, producing an unhandled rejection.
updateLastConnected now swallows unreadable-storage failures internally since
it's a best-effort timestamp.

P2 (soft-read): loadPendingHostCredentialCleanup now reports storageUnreadable
instead of pretending the queue is empty, and Settings surfaces a
"couldn't check cleanup status — retry to be safe" affordance rather than
hiding the section when the durable queue can't be read.

Tests: dual-fault fallback + no-clobber, storageUnreadable reporting,
fallback self-heal on late delete success, and updateLastConnected non-throw.
This commit is contained in:
Jinjing 2026-07-11 21:42:03 -07:00 committed by GitHub
parent 01dcbcb007
commit 92ea918b63
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 172 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
mobile/app/settings.tsx
View File

@ -25,7 +25,7 @@ import {
} from 'lucide-react-native'
import { colors, radii, spacing, typography } from '../src/theme/mobile-theme'
import {
loadPendingHostCredentialCleanupIds,
loadPendingHostCredentialCleanup,
subscribePendingHostCredentialCleanup
} from '../src/transport/host-credential-cleanup'
import { retryPendingHostCredentialCleanup } from '../src/transport/host-store'
@ -34,6 +34,7 @@ export default function SettingsScreen() {
const router = useRouter()
const insets = useSafeAreaInsets()
const [pendingCredentialIds, setPendingCredentialIds] = useState<string[]>([])
const [credentialStorageUnreadable, setCredentialStorageUnreadable] = useState(false)
const [retryingCredentialCleanup, setRetryingCredentialCleanup] = useState(false)
const [credentialRetryFailed, setCredentialRetryFailed] = useState(false)
const credentialRefreshGenerationRef = useRef(0)
@ -44,12 +45,13 @@ export default function SettingsScreen() {
setCredentialRetryFailed(false)
const refresh = () => {
const generation = ++credentialRefreshGenerationRef.current
void loadPendingHostCredentialCleanupIds().then((ids) => {
void loadPendingHostCredentialCleanup().then((state) => {
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) {
setPendingCredentialIds(state.ids)
setCredentialStorageUnreadable(state.storageUnreadable)
// Why: neutral copy once the queue is confirmed empty so a later
// pending set does not inherit a previous Retry failure message.
if (state.ids.length === 0 && !state.storageUnreadable) {
setCredentialRetryFailed(false)
}
}
@ -74,7 +76,8 @@ export default function SettingsScreen() {
try {
const result = await retryPendingHostCredentialCleanup()
setPendingCredentialIds(result.remainingIds)
setCredentialRetryFailed(result.remainingIds.length > 0)
setCredentialStorageUnreadable(result.storageUnreadable)
setCredentialRetryFailed(result.remainingIds.length > 0 || result.storageUnreadable)
} catch {
setCredentialRetryFailed(true)
} finally {
@ -83,6 +86,10 @@ export default function SettingsScreen() {
}, [retryingCredentialCleanup])
const pendingCredentialCount = pendingCredentialIds.length
// Why: show the cleanup card whenever cleanup is pending OR the durable queue
// is unreadable — an unreadable queue can hide an orphaned token, so keep a
// retry affordance rather than a silently-empty (hidden) section.
const showCredentialCleanup = pendingCredentialCount > 0 || credentialStorageUnreadable
return (
<View style={[styles.container, { paddingTop: insets.top + spacing.sm }]}>
@ -153,7 +160,7 @@ export default function SettingsScreen() {
</Pressable>
</View>
{pendingCredentialCount > 0 ? (
{showCredentialCleanup ? (
<View style={[styles.section, styles.sectionSpacer]}>
<View style={styles.credentialCleanupRow}>
<KeyRound size={16} color={colors.statusAmber} />
@ -162,7 +169,9 @@ export default function SettingsScreen() {
<Text accessibilityLiveRegion="polite" style={styles.rowHint}>
{credentialRetryFailed
? "Cleanup still couldn't be confirmed. Try again later."
: `Couldn't confirm cleanup for ${pendingCredentialCount} credential${pendingCredentialCount === 1 ? '' : 's'} on this device.`}
: pendingCredentialCount > 0
? `Couldn't confirm cleanup for ${pendingCredentialCount} credential${pendingCredentialCount === 1 ? '' : 's'} on this device.`
: "Couldn't check cleanup status on this device. Retry to be safe."}
</Text>
</View>
<Pressable

56
mobile/src/transport/host-credential-cleanup.test.ts
View File

@ -1,6 +1,7 @@
import AsyncStorage from '@react-native-async-storage/async-storage'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import {
loadPendingHostCredentialCleanup,
loadPendingHostCredentialCleanupIds,
resetHostCredentialCleanupForTests,
retryPendingHostCredentialCleanups,
@ -167,7 +168,8 @@ describe('host credential cleanup', () => {
await expect(retryPendingHostCredentialCleanups(deleteCredential)).resolves.toEqual({
clearedCount: 1,
remainingIds: []
remainingIds: [],
storageUnreadable: false
})
expect(deleteCredential).toHaveBeenCalledTimes(2)
})
@ -182,24 +184,68 @@ describe('host credential cleanup', () => {
expect(deleteCredential).not.toHaveBeenCalled()
await expect(retryPendingHostCredentialCleanups(deleteCredential)).resolves.toEqual({
clearedCount: 1,
remainingIds: ['host-1']
remainingIds: ['host-1'],
storageUnreadable: false
})
expect(deleteCredential).toHaveBeenCalledTimes(2)
})
it('does not wipe existing pending ids when storage read fails during mutation', async () => {
it('surfaces a fallback handle without clobbering durable ids when the queue write fails', async () => {
storedPendingIds = ['host-a']
const deleteCredential = vi.fn().mockRejectedValue(new Error('keychain unavailable'))
const listener = vi.fn()
const unsubscribe = subscribePendingHostCredentialCleanup(listener)
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).
// Durable queue is untouched (not clobbered); the failed-to-record host-b is
// still surfaced via the session-scoped fallback so its orphaned token keeps
// a retry affordance instead of being silently lost.
expect(deleteCredential).toHaveBeenCalledOnce()
await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-a'])
expect(listener).toHaveBeenCalled()
await expect(loadPendingHostCredentialCleanupIds()).resolves.toEqual(['host-a', 'host-b'])
expect(asyncStorageMock.setItem).not.toHaveBeenCalled()
unsubscribe()
})
it('reports storageUnreadable when the durable queue cannot be read', async () => {
storedPendingIds = ['host-a']
readShouldFail = true
await expect(loadPendingHostCredentialCleanup()).resolves.toEqual({
ids: [],
storageUnreadable: true
})
})
it('clears the fallback handle once the native delete finally succeeds', async () => {
let resolveDelete: (() => void) | null = null
const deleteCredential = vi.fn(
() =>
new Promise<void>((resolve) => {
resolveDelete = resolve
})
)
readShouldFail = true
await scheduleHostCredentialCleanup('host-b', deleteCredential, 1_000)
await vi.waitFor(() => expect(deleteCredential).toHaveBeenCalledOnce())
readShouldFail = false
await expect(loadPendingHostCredentialCleanup()).resolves.toEqual({
ids: ['host-b'],
storageUnreadable: false
})
resolveDelete?.()
await vi.waitFor(async () => {
await expect(loadPendingHostCredentialCleanup()).resolves.toEqual({
ids: [],
storageUnreadable: false
})
})
})
it('cleans a new credential when the same host id is paired and removed again', async () => {

99
mobile/src/transport/host-credential-cleanup.ts
View File

@ -8,11 +8,43 @@ type CleanupAttemptResult = 'cleared' | 'pending'
type CleanupOutcome = 'cleared' | 'failed' | 'timed-out'
type PendingIdsRead = { ok: true; ids: string[] } | { ok: false }
export type PendingHostCredentialCleanup = {
ids: string[]
// Why: the durable AsyncStorage queue could not be read. Settings surfaces a
// "pending unknown / retry to be safe" state instead of a silently-empty
// (hidden) section so an orphaned keychain token keeps a recovery affordance.
storageUnreadable: boolean
}
let pendingMutation: Promise<void> = 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<string, Promise<void>>()
// Why: when the durable queue write fails we still need a recovery handle for a
// failed keychain delete. Keep the hostId here (session-scoped) so Settings can
// surface it and offer a retry; cleared once the native delete confirms.
const unrecordedPendingIds = new Set<string>()
function notifyPendingListeners(): void {
for (const listener of pendingListeners) {
listener()
}
}
function markUnrecordedPending(hostId: string): void {
if (unrecordedPendingIds.has(hostId)) {
return
}
unrecordedPendingIds.add(hostId)
notifyPendingListeners()
}
function clearUnrecordedPending(hostId: string): void {
if (unrecordedPendingIds.delete(hostId)) {
notifyPendingListeners()
}
}
function parsePendingIds(raw: string): string[] | null {
try {
@ -48,9 +80,17 @@ async function readPendingIdsForMutation(): Promise<PendingIdsRead> {
}
}
async function readPendingIdsSoft(): Promise<string[]> {
async function loadPendingCleanupState(): Promise<PendingHostCredentialCleanup> {
await pendingMutation
const result = await readPendingIdsForMutation()
return result.ok ? result.ids : []
const fallback = [...unrecordedPendingIds]
if (!result.ok) {
// Why: durable queue unreadable — only the session-scoped fallback is
// known. Report unreadable so callers can surface a retry rather than
// pretend the queue is empty.
return { ids: [...new Set(fallback)], storageUnreadable: true }
}
return { ids: [...new Set([...result.ids, ...fallback])], storageUnreadable: false }
}
async function mutatePendingIds(update: (ids: string[]) => string[]): Promise<void> {
@ -64,9 +104,7 @@ async function mutatePendingIds(update: (ids: string[]) => string[]): Promise<vo
return
}
await AsyncStorage.setItem(PENDING_STORAGE_KEY, JSON.stringify(next))
for (const listener of pendingListeners) {
listener()
}
notifyPendingListeners()
})
pendingMutation = mutation.catch(() => {})
return mutation
@ -115,11 +153,12 @@ function startOrJoinDelete(hostId: string, deleteCredential: DeleteHostCredentia
return cleanup
}
async function recordCleanupIntent(hostId: string): Promise<void> {
async function recordCleanupIntent(hostId: string): Promise<boolean> {
try {
await addPendingId(hostId)
return true
} catch {
// Best-effort native delete can still proceed without a durable row.
return false
}
}
@ -130,9 +169,14 @@ async function confirmNativeCleanup(
): Promise<CleanupAttemptResult> {
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).
// still clears the queue entry (including after timed-out returns). Clears the
// session-scoped fallback too, so a durable-write-failed intent stops being
// surfaced once the native delete finally lands.
const clearWhenDeleted = cleanup.then(
() => removePendingId(hostId).catch(() => undefined),
() => {
clearUnrecordedPending(hostId)
return removePendingId(hostId).catch(() => undefined)
},
() => undefined
)
const outcome = await observeCleanup(cleanup, timeoutMs)
@ -152,9 +196,12 @@ async function confirmNativeCleanup(
return 'pending'
}
export async function loadPendingHostCredentialCleanup(): Promise<PendingHostCredentialCleanup> {
return loadPendingCleanupState()
}
export async function loadPendingHostCredentialCleanupIds(): Promise<string[]> {
await pendingMutation
return readPendingIdsSoft()
return (await loadPendingCleanupState()).ids
}
export function subscribePendingHostCredentialCleanup(listener: () => void): () => void {
@ -163,31 +210,40 @@ export function subscribePendingHostCredentialCleanup(listener: () => void): ()
}
/**
* Await only durable intent (AsyncStorage). Native keychain delete is
* fire-and-forget so removeHost never blocks on SecureStore.
* Record cleanup intent, then fire-and-forget the native keychain delete so
* removeHost never blocks on SecureStore. If the durable intent write fails,
* keep a session-scoped recovery handle so a failed keychain delete still
* surfaces in Settings instead of orphaning the token with no retry affordance.
*/
export async function scheduleHostCredentialCleanup(
hostId: string,
deleteCredential: DeleteHostCredential,
timeoutMs = CLEANUP_CONFIRM_TIMEOUT_MS
): Promise<void> {
await recordCleanupIntent(hostId)
const recorded = await recordCleanupIntent(hostId)
if (!recorded) {
// Why: the only durable recovery handle failed to persist. Hold an in-memory
// one so Settings can still surface + retry; confirmNativeCleanup clears it
// if the native delete lands. removeHost stays non-blocking (freeze fix).
markUnrecordedPending(hostId)
}
void confirmNativeCleanup(hostId, deleteCredential, timeoutMs).catch(() => {})
}
export async function retryPendingHostCredentialCleanups(
deleteCredential: DeleteHostCredential
): Promise<{ clearedCount: number; remainingIds: string[] }> {
const ids = await loadPendingHostCredentialCleanupIds()
): Promise<{ clearedCount: number; remainingIds: string[]; storageUnreadable: boolean }> {
const pending = await loadPendingCleanupState()
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))
// Why: these ids are already durable (or a session-scoped fallback). Re-adding
// intent can race a late success and recreate a ghost row after deletion.
pending.ids.map((id) => confirmNativeCleanup(id, deleteCredential, CLEANUP_CONFIRM_TIMEOUT_MS))
)
const remainingIds = await loadPendingHostCredentialCleanupIds()
const remaining = await loadPendingCleanupState()
return {
clearedCount: outcomes.filter((outcome) => outcome === 'cleared').length,
remainingIds
remainingIds: remaining.ids,
storageUnreadable: remaining.storageUnreadable
}
}
@ -195,5 +251,6 @@ export async function retryPendingHostCredentialCleanups(
export function resetHostCredentialCleanupForTests(): void {
inflightDeletes.clear()
pendingListeners.clear()
unrecordedPendingIds.clear()
pendingMutation = Promise.resolve()
}

9
mobile/src/transport/host-store.test.ts
View File

@ -164,4 +164,13 @@ describe('host-store list mutations', () => {
expect(asyncStorageMock.setItem).not.toHaveBeenCalled()
expect(storedHostsRaw).toBe('{')
})
it('resolves instead of rejecting when updateLastConnected hits unreadable storage', async () => {
// Why: callers fire updateLastConnected with `void`; a rejection here would
// surface as an unhandled promise rejection rather than a caught error.
storedHostsRaw = '{'
await expect(updateLastConnected(HOST_ONE.id)).resolves.toBeUndefined()
expect(asyncStorageMock.setItem).not.toHaveBeenCalled()
expect(storedHostsRaw).toBe('{')
})
})

25
mobile/src/transport/host-store.ts
View File

@ -232,6 +232,7 @@ export async function removeHost(hostId: string): Promise<void> {
export async function retryPendingHostCredentialCleanup(): Promise<{
clearedCount: number
remainingIds: string[]
storageUnreadable: boolean
}> {
return retryPendingHostCredentialCleanups(deleteDeviceToken)
}
@ -255,15 +256,21 @@ export async function getNextHostName(): Promise<string> {
}
export async function updateLastConnected(hostId: string): Promise<void> {
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
})
try {
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
})
} catch {
// Why: last-connected is a best-effort timestamp and callers fire it with
// `void`. Swallow unreadable-storage failures so they don't surface as an
// unhandled promise rejection.
}
}
/** Test-only: drain module mutation chain between cases. */