From 85d7cf3cc1e3737a7c00db2f3847ef8dc41daa83 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 13 Sep 2026 18:23:13 -0400 Subject: [PATCH] fix(mobile): preserve delivery ambiguity across transport cutover (#20280) * fix(mobile): preserve delivery ambiguity across transport cutover Let physical close settle requests and retain its error as the cutover cause, copying only an existing delivery-unknown mark. Pin sent and unsent caller outcomes and both cutover predicate carriers. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): pin the RpcClient.close() settlement contract close() was declared `() => void` with no stated obligation. That was harmless while migrateTo rejected pendings itself; now that it does not, close() is the retiring generation's only settlement path, so a type-compatible implementation that leaves a request pending strands its caller for good. States the obligation on the declaration and pins it for both trackers the real implementations reject through. Dropping the delivery-unknown flag, dropping the relay mark, or leaving pendings in the map each fail a test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): read the cutover cause without a type assertion main's new casting gate rejects `(error as Error).cause`; narrow instead so the assertion still distinguishes a missing cause from an unmarked one. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../rpc-client-delivery-ambiguity.test.ts | 53 ++++++++++++++++++- mobile/src/transport/rpc-client.ts | 13 +++++ .../stable-logical-rpc-client.test.ts | 1 + .../transport/stable-logical-rpc-client.ts | 29 +++++----- .../worktree/home-host-worktree-fetch.test.ts | 6 ++- 5 files changed, 84 insertions(+), 18 deletions(-) diff --git a/mobile/src/transport/rpc-client-delivery-ambiguity.test.ts b/mobile/src/transport/rpc-client-delivery-ambiguity.test.ts index 5cdaf75b945..29456c2eaa1 100644 --- a/mobile/src/transport/rpc-client-delivery-ambiguity.test.ts +++ b/mobile/src/transport/rpc-client-delivery-ambiguity.test.ts @@ -1,6 +1,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { connect } from './rpc-client' import { isRpcDeliveryUnknown } from './rpc-delivery-ambiguity' +import { + createStableLogicalRpcClient, + isLogicalClientCutoverError, + LogicalClientCutoverError +} from './stable-logical-rpc-client' vi.mock('./e2ee', () => ({ generateKeyPair: () => ({ @@ -72,7 +77,7 @@ function hasSentRequest(socket: MockWebSocket, method: string): boolean { function connectAuthenticated(): { client: ReturnType; socket: MockWebSocket } { const client = connect('ws://desktop.invalid', 'token', 'server-key') - const socket = mockSockets[0]! + const socket = mockSockets[mockSockets.length - 1]! socket.open() socket.receive(JSON.stringify({ type: 'e2ee_ready' })) socket.receive('encrypted:{"type":"e2ee_authenticated"}') @@ -94,6 +99,52 @@ describe('mobile rpc-client delivery ambiguity marking', () => { globalThis.WebSocket = originalWebSocket }) + it.each([true, false])( + 'preserves physical delivery evidence at the cutover caller (sent=%s)', + async (sent) => { + const physical = sent + ? connectAuthenticated() + : { + client: connect('ws://desktop.invalid', 'token', 'server-key'), + socket: mockSockets[0]! + } + const client = createStableLogicalRpcClient(physical.client, 'lan') + const replacement = connectAuthenticated() + const requestError = client + .sendRequest('worktree.create', { name: 'new' }) + .catch((error: unknown) => error) + await Promise.resolve() + expect(hasSentRequest(physical.socket, 'worktree.create')).toBe(sent) + + await client.migrateTo(replacement.client, 'relay') + + const error = await requestError + expect(isLogicalClientCutoverError(error)).toBe(true) + expect(isRpcDeliveryUnknown(error)).toBe(sent) + expect(error).toBeInstanceOf(LogicalClientCutoverError) + expect(isRpcDeliveryUnknown(error instanceof Error ? error.cause : null)).toBe(sent) + expect(hasSentRequest(replacement.socket, 'worktree.create')).toBe(false) + expect( + physical.socket.sent.filter((payload) => payload.includes('worktree.create')) + ).toHaveLength(sent ? 1 : 0) + client.close() + } + ) + + it('recognizes a cutover by class even when its message changes', () => { + const error = new LogicalClientCutoverError() + error.message = 'wrapped migration' + expect(isLogicalClientCutoverError(error)).toBe(true) + }) + + it('recognizes a cutover message from another bundle copy', () => { + expect(isLogicalClientCutoverError(new Error('RPC interrupted by connection migration'))).toBe( + true + ) + expect(isLogicalClientCutoverError(new Error('Client closed'))).toBe(false) + expect(isLogicalClientCutoverError('RPC interrupted by connection migration')).toBe(false) + }) + it('marks in-flight requests as delivery-unknown when the socket drops', async () => { const { client, socket } = connectAuthenticated() const requestError = client.sendRequest('terminal.send', { terminal: 't' }).then( diff --git a/mobile/src/transport/rpc-client.ts b/mobile/src/transport/rpc-client.ts index 38941483c4d..05fdbbac8fa 100644 --- a/mobile/src/transport/rpc-client.ts +++ b/mobile/src/transport/rpc-client.ts @@ -32,6 +32,19 @@ export type RpcClient = UnvalidatedRpcRequestPort & { getLastInboundAt?: () => number | null onStateChange: (listener: (state: ConnectionState) => void) => () => void notifyForeground: (reason?: ForegroundNudgeReason) => void + /** + * Must settle every pending `sendRequest` promise before returning. + * + * `StableLogicalRpcClient.migrateTo` no longer rejects pendings itself — the physical + * sender is the only layer that knows whether a request reached the wire, so + * `previous.close()` is the sole settlement path for the retiring generation. An + * implementation that leaves a request pending strands its caller for good. + * + * Requests that did reach the wire must reject with a delivery-unknown error + * (`markRpcDeliveryUnknown`), since the host may already have executed them. Pinned + * against the real clients in `rpc-client-delivery-ambiguity.test.ts` (direct) and + * `mobile-relay-rpc-session.test.ts` (relay) — a new implementation needs its own case. + */ close: () => void } diff --git a/mobile/src/transport/stable-logical-rpc-client.test.ts b/mobile/src/transport/stable-logical-rpc-client.test.ts index cbbf01ada2a..6a22b3654a2 100644 --- a/mobile/src/transport/stable-logical-rpc-client.test.ts +++ b/mobile/src/transport/stable-logical-rpc-client.test.ts @@ -90,6 +90,7 @@ describe('stable logical RPC client', () => { const nextSession = new FakeSession('connecting') const pending = deferred() oldSession.sendRequest.mockReturnValue(pending.promise) + oldSession.close.mockImplementation(() => pending.reject(new Error('Client closed'))) nextSession.sendRequest.mockResolvedValue(success('next')) const client = createStableLogicalRpcClient(oldSession, 'lan') const stream = vi.fn() diff --git a/mobile/src/transport/stable-logical-rpc-client.ts b/mobile/src/transport/stable-logical-rpc-client.ts index 3a896af6f5b..769481aa80c 100644 --- a/mobile/src/transport/stable-logical-rpc-client.ts +++ b/mobile/src/transport/stable-logical-rpc-client.ts @@ -7,12 +7,16 @@ import { import { waitForAuthenticated } from './replacement-session-authentication' import { projectMobileRpcRequestParams } from './mobile-rpc-request-projection' import { LogicalClientConnectionPath } from './logical-client-connection-path' +import { isRpcDeliveryUnknown, markRpcDeliveryUnknown } from './rpc-delivery-ambiguity' export type MobileConnectionPath = 'lan' | 'tailscale' | 'relay' export class LogicalClientCutoverError extends Error { - constructor() { - super('RPC interrupted by connection migration') + constructor(cause?: unknown) { + super('RPC interrupted by connection migration', { cause }) + if (isRpcDeliveryUnknown(cause)) { + markRpcDeliveryUnknown(this) + } } } @@ -33,10 +37,6 @@ type SubscriptionRecord = { cancelled: boolean } -type PendingRequest = { - reject: (error: Error) => void -} - export type StableLogicalRpcClient = RpcClient & { migrateTo( session: RpcClient, @@ -75,7 +75,6 @@ export function createStableLogicalRpcClient( let nextSubscriptionId = 0 let activeStateUnsubscribe: (() => void) | null = null const subscriptions = new Map() - const pendingRequests = new Set() const stateListeners = new Set<(state: ConnectionState) => void>() let state = initialSession.getState() const connectionPath = new LogicalClientConnectionPath(() => state === 'connected') @@ -90,22 +89,23 @@ export function createStableLogicalRpcClient( if (suspended) { return Promise.reject(new Error('Client suspended')) } + const requestGeneration = generation const session = activeSession return new Promise((resolve, reject) => { - const pending = { reject } - pendingRequests.add(pending) void session .sendRequest(method, projectMobileRpcRequestParams(method, params), options) .then( (response) => { - pendingRequests.delete(pending) // A correlated response is definitive even if close/cutover won the // callback race after the physical promise had already settled. resolve(response) }, (error: unknown) => { - pendingRequests.delete(pending) - reject(error) + // Why: the retiring physical session settles this, so keep its error as the + // cause — it is the only evidence of whether the frame reached the wire. + reject( + requestGeneration !== generation ? new LogicalClientCutoverError(error) : error + ) } ) }) @@ -260,15 +260,12 @@ export function createStableLogicalRpcClient( suspended = false previousStateUnsubscribe?.() bindActiveState(nextSession, nextGeneration) - for (const pending of pendingRequests) { - pending.reject(new LogicalClientCutoverError()) - } - pendingRequests.clear() state = nextSession.getState() connectionPath.clearAfterConnected() for (const listener of stateListeners) { listener(state) } + // Only the physical sender knows whether a pending request reached the wire. previous.close() }, diff --git a/mobile/src/worktree/home-host-worktree-fetch.test.ts b/mobile/src/worktree/home-host-worktree-fetch.test.ts index 5799de96366..6238554769a 100644 --- a/mobile/src/worktree/home-host-worktree-fetch.test.ts +++ b/mobile/src/worktree/home-host-worktree-fetch.test.ts @@ -39,7 +39,11 @@ function fakeSession(): FakeSession { getLastConnectedAt: () => null, onStateChange: () => () => {}, notifyForeground: () => {}, - close: () => {} + close: () => { + for (const settle of pending.splice(0)) { + settle(new Error('Client closed')) + } + } } } return fake