From 8e00b065bdf0839aca16ebdb07f409f991c5674c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 1 Aug 2026 12:43:14 -0700 Subject: [PATCH] fix(mobile): close reconnect review gaps --- .../app/h/[hostId]/session/[worktreeId].tsx | 2 +- .../client-context-all-host-clients.ts | 4 +-- mobile/src/transport/client-context.test.ts | 28 ++++++++++------ ...sting-host-relay-routing-lifecycle.test.ts | 31 ++++++++++++++++++ .../force-reconnect-rpc-health.test.ts | 32 ++++++++++++------- .../transport/force-reconnect-rpc-health.ts | 14 +++++--- .../src/transport/host-profile-publication.ts | 1 + .../transport/mobile-endpoint-lifecycle.ts | 10 +++++- ...obile-relay-direct-upgrade-journal.test.ts | 23 +++++++++++++ .../mobile-relay-direct-upgrade-journal.ts | 5 +-- .../rpc-client-request-deadline.test.ts | 2 +- .../stable-logical-rpc-client.test.ts | 14 +++++--- 12 files changed, 127 insertions(+), 39 deletions(-) diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index f5c0eb0f127..83b2fe04f7f 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -4457,7 +4457,7 @@ export default function SessionScreen() { accessibilityRole={showConnectionRetry ? 'button' : undefined} accessibilityLabel={showConnectionRetry ? 'Reconnect to desktop' : undefined} > - + {terminalSummary} diff --git a/mobile/src/transport/client-context-all-host-clients.ts b/mobile/src/transport/client-context-all-host-clients.ts index e5d33918768..7519e343408 100644 --- a/mobile/src/transport/client-context-all-host-clients.ts +++ b/mobile/src/transport/client-context-all-host-clients.ts @@ -31,12 +31,12 @@ export function useAllHostClients(hostIds: string[]) { }, [key]) return useMemo(() => { - const clients: Array<{ + const clients: { hostId: string client: RpcClient state: ConnectionState path: MobileConnectionPath - }> = [] + }[] = [] for (const id of hostIds) { const found = context.getAllClients().find((entry) => entry.hostId === id) if (found) { diff --git a/mobile/src/transport/client-context.test.ts b/mobile/src/transport/client-context.test.ts index 10e7790b3c8..9d8e3d1a51e 100644 --- a/mobile/src/transport/client-context.test.ts +++ b/mobile/src/transport/client-context.test.ts @@ -289,11 +289,15 @@ describe('useHostClient', () => { }) expect(stale.closeMock).toHaveBeenCalledOnce() - expect(fresh.sendRequest).toHaveBeenCalledWith('status.get', undefined, { - timeoutMs: expect.any(Number), - budgetSpansConnect: true, - strictDeadline: true - }) + expect(fresh.sendRequest).toHaveBeenCalledWith( + 'worktree.list', + { limit: 1 }, + { + timeoutMs: expect.any(Number), + budgetSpansConnect: true, + strictDeadline: true + } + ) expect(completed).toBe(false) resolveHealthCheck?.() @@ -623,11 +627,15 @@ describe('useHostClient', () => { expect(connectMock.mock.calls[2]?.[0]).toEqual(updatedHost) expect(oldReplacement.closeMock).toHaveBeenCalledOnce() - expect(newReplacement.sendRequest).toHaveBeenCalledWith('status.get', undefined, { - timeoutMs: expect.any(Number), - budgetSpansConnect: true, - strictDeadline: true - }) + expect(newReplacement.sendRequest).toHaveBeenCalledWith( + 'worktree.list', + { limit: 1 }, + { + timeoutMs: expect.any(Number), + budgetSpansConnect: true, + strictDeadline: true + } + ) harness.unmount() }) diff --git a/mobile/src/transport/existing-host-relay-routing-lifecycle.test.ts b/mobile/src/transport/existing-host-relay-routing-lifecycle.test.ts index 9bfe3f06da9..7bcd9b8adee 100644 --- a/mobile/src/transport/existing-host-relay-routing-lifecycle.test.ts +++ b/mobile/src/transport/existing-host-relay-routing-lifecycle.test.ts @@ -219,6 +219,37 @@ describe('existing host relay endpoint lifecycle publication', () => { expect(events).toEqual(['publication-started', 'publication-finished', 'removed']) }) + it('fences a queued stale publication before it mutates credentials', async () => { + let releaseFirstPublication: (() => void) | null = null + let markFirstPublicationStarted: (() => void) | null = null + const firstPublicationStarted = new Promise((resolve) => { + markFirstPublicationStarted = resolve + }) + const firstPublication = publishHostProfileTransaction( + HOST, + async () => { + markFirstPublicationStarted?.() + await new Promise((resolve) => { + releaseFirstPublication = resolve + }) + }, + async () => {} + ) + await firstPublicationStarted + + const staleCredentialWrite = vi.fn(async () => {}) + const stalePublication = publishHostProfileTransaction( + { ...HOST, deviceToken: 'token-2' }, + staleCredentialWrite, + async () => {} + ) + releaseFirstPublication?.() + + await firstPublication + await expect(stalePublication).rejects.toThrow('host profile publication was retired') + expect(staleCredentialWrite).not.toHaveBeenCalled() + }) + it('rejects an old credential write that starts during same-host re-pair publication', async () => { let releaseReplacement: (() => void) | null = null let markReplacementStarted: (() => void) | null = null diff --git a/mobile/src/transport/force-reconnect-rpc-health.test.ts b/mobile/src/transport/force-reconnect-rpc-health.test.ts index a9043a937cd..398385d3210 100644 --- a/mobile/src/transport/force-reconnect-rpc-health.test.ts +++ b/mobile/src/transport/force-reconnect-rpc-health.test.ts @@ -41,11 +41,15 @@ describe('Force Reconnect RPC health', () => { expect(outcome).toBe('stalled') expect(sendRequest).toHaveBeenCalledTimes(2) - expect(sendRequest).toHaveBeenLastCalledWith('status.get', undefined, { - timeoutMs: 1_000, - budgetSpansConnect: true, - strictDeadline: true - }) + expect(sendRequest).toHaveBeenLastCalledWith( + 'worktree.list', + { limit: 1 }, + { + timeoutMs: 1_000, + budgetSpansConnect: true, + strictDeadline: true + } + ) }) it('waits through a transient authorization retry', async () => { @@ -67,22 +71,26 @@ describe('Force Reconnect RPC health', () => { expect(sendRequest).toHaveBeenCalledTimes(2) }) - it('rejects a successful probe until a real application response clears the shared latch', async () => { + it('clears the shared latch with an application-level replacement probe', async () => { const responsiveness = new RpcApplicationResponsiveness() responsiveness.recordTimeout('browser.screenshot', 123) - const sendRequest = vi.fn(async () => ({ id: 'rpc-1', ok: true, result: {} }) as RpcResponse) + const sendRequest = vi.fn(async (method: string) => { + responsiveness.recordResponse(method) + return { id: 'rpc-1', ok: true, result: {} } as RpcResponse + }) const client = { sendRequest, getState: () => 'connected' as const, getRpcUnresponsiveSince: () => responsiveness.getUnresponsiveSince() } as unknown as RpcClient - await expect(verifyForceReconnectRpcHealth(client)).rejects.toThrow( - 'Application RPC channel is still not responding' - ) - responsiveness.recordResponse('worktree.list') await expect(verifyForceReconnectRpcHealth(client)).resolves.toBeUndefined() - expect(sendRequest).toHaveBeenCalledTimes(2) + expect(sendRequest).toHaveBeenCalledWith( + 'worktree.list', + { limit: 1 }, + expect.objectContaining({ strictDeadline: true }) + ) + expect(responsiveness.getUnresponsiveSince()).toBeNull() }) it('fails when authorization retries reach auth-failed', async () => { diff --git a/mobile/src/transport/force-reconnect-rpc-health.ts b/mobile/src/transport/force-reconnect-rpc-health.ts index fe09257cac4..dc857efad97 100644 --- a/mobile/src/transport/force-reconnect-rpc-health.ts +++ b/mobile/src/transport/force-reconnect-rpc-health.ts @@ -15,11 +15,15 @@ export async function verifyForceReconnectRpcHealth( throw lastError ?? new Error('Force Reconnect health check timed out') } try { - await client.sendRequest('status.get', undefined, { - timeoutMs, - budgetSpansConnect: true, - strictDeadline: true - }) + await client.sendRequest( + 'worktree.list', + { limit: 1 }, + { + timeoutMs, + budgetSpansConnect: true, + strictDeadline: true + } + ) if (client.getRpcUnresponsiveSince?.() != null) { throw new Error('Application RPC channel is still not responding') } diff --git a/mobile/src/transport/host-profile-publication.ts b/mobile/src/transport/host-profile-publication.ts index 48763b822db..9668ffefcc9 100644 --- a/mobile/src/transport/host-profile-publication.ts +++ b/mobile/src/transport/host-profile-publication.ts @@ -81,6 +81,7 @@ export function publishHostProfileTransaction( ): Promise { const publicationRevision = getHostProfilePublicationRevision(host.id) return serializeHostProfilePublication(host.id, async () => { + requirePublicationRevision(host.id, publicationRevision) await beforeHostSave?.() requirePublicationRevision(host.id, publicationRevision) await saveHost(host) diff --git a/mobile/src/transport/mobile-endpoint-lifecycle.ts b/mobile/src/transport/mobile-endpoint-lifecycle.ts index 2ece7890cdd..195948b238c 100644 --- a/mobile/src/transport/mobile-endpoint-lifecycle.ts +++ b/mobile/src/transport/mobile-endpoint-lifecycle.ts @@ -14,6 +14,7 @@ import { } from './existing-host-relay-routing' import { beginHostEndpointPublicationLifecycle, + getHostEndpointPublicationLifecycle, type HostEndpointPublicationLifecycle } from './host-profile-publication' import { upgradeDirectMobileRelay } from './mobile-relay-direct-upgrade' @@ -78,7 +79,14 @@ export function startMobileEndpointLifecycle( if (initialHost.relay) { // Why: relay publication supersedes any committed journal retained by the retired direct owner. - void retireMobileRelayDirectUpgradeJournalForRelayHost(initialHost.id).catch(() => {}) + void retireMobileRelayDirectUpgradeJournalForRelayHost(initialHost.id, () => { + const current = getHostEndpointPublicationLifecycle(initialHost.id) + return ( + !stopped && + current.generation === endpointLifecycle.generation && + current.profileRevision === endpointLifecycle.profileRevision + ) + }).catch(() => {}) owner = createSupervisor( logical, initialHost, diff --git a/mobile/src/transport/mobile-relay-direct-upgrade-journal.test.ts b/mobile/src/transport/mobile-relay-direct-upgrade-journal.test.ts index bdd6070a673..7b29d514736 100644 --- a/mobile/src/transport/mobile-relay-direct-upgrade-journal.test.ts +++ b/mobile/src/transport/mobile-relay-direct-upgrade-journal.test.ts @@ -37,4 +37,27 @@ describe('mobile relay direct upgrade journal retirement', () => { JSON.stringify(journal) ) }) + + it('does not clear a replacement journal after the owning lifecycle stops', async () => { + const journal = createMobileRelayDirectUpgradeJournal('host-1', (length) => + new Uint8Array(length).fill(8) + ) + let resolveRead: ((raw: string) => void) | null = null + keychain.readPairingKeychainItem.mockReturnValueOnce( + new Promise((resolve) => { + resolveRead = resolve + }) + ) + let active = true + const retirement = retireMobileRelayDirectUpgradeJournalForRelayHost( + journal.hostId, + () => active + ) + + active = false + resolveRead?.(JSON.stringify(journal)) + await retirement + + expect(keychain.deletePairingKeychainItemIfMatches).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/transport/mobile-relay-direct-upgrade-journal.ts b/mobile/src/transport/mobile-relay-direct-upgrade-journal.ts index e75a4daf313..34206304a42 100644 --- a/mobile/src/transport/mobile-relay-direct-upgrade-journal.ts +++ b/mobile/src/transport/mobile-relay-direct-upgrade-journal.ts @@ -82,10 +82,11 @@ export function deleteMobileRelayDirectUpgradeJournalIfCurrent( } export async function retireMobileRelayDirectUpgradeJournalForRelayHost( - hostId: string + hostId: string, + shouldRetire: () => boolean = () => true ): Promise { const journal = await readMobileRelayDirectUpgradeJournal(hostId) - if (journal) { + if (journal && shouldRetire()) { await deleteMobileRelayDirectUpgradeJournalIfCurrent(journal) } } diff --git a/mobile/src/transport/rpc-client-request-deadline.test.ts b/mobile/src/transport/rpc-client-request-deadline.test.ts index be80d692b4b..7d029dced6e 100644 --- a/mobile/src/transport/rpc-client-request-deadline.test.ts +++ b/mobile/src/transport/rpc-client-request-deadline.test.ts @@ -442,7 +442,7 @@ describe('mobile rpc-client request deadline', () => { await vi.advanceTimersByTimeAsync(1) await settled - expect(outcome).toBe('Request timed out: status.get') + expect(outcome).toBe('Request timed out: worktree.list') client.close() }) diff --git a/mobile/src/transport/stable-logical-rpc-client.test.ts b/mobile/src/transport/stable-logical-rpc-client.test.ts index 25ff35438de..bff1daa46a5 100644 --- a/mobile/src/transport/stable-logical-rpc-client.test.ts +++ b/mobile/src/transport/stable-logical-rpc-client.test.ts @@ -116,11 +116,15 @@ describe('stable logical RPC client', () => { await client.migrateTo(nextSession, 'relay') await expect(verifying).resolves.toBeUndefined() - expect(nextSession.sendRequest).toHaveBeenCalledWith('status.get', undefined, { - timeoutMs: expect.any(Number), - budgetSpansConnect: true, - strictDeadline: true - }) + expect(nextSession.sendRequest).toHaveBeenCalledWith( + 'worktree.list', + { limit: 1 }, + { + timeoutMs: expect.any(Number), + budgetSpansConnect: true, + strictDeadline: true + } + ) pending.resolve(success('late')) })