From 643aaa007afca2bc3823ebe28d2eb920023dec8a Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 11 Oct 2026 22:40:24 -0400 Subject: [PATCH] fix(mobile): keep polling the server list and read a server's reachability from relay A hosts-list read slower than the poll stopped the feed for good, since the next tick was scheduled when the read started; it is now scheduled from the finish. A server workspace now reads connected only when the desktop relays to it, so a server with no status yet is not shown up. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../scripts/verify-mobile-web-app-bundle.mjs | 13 +++-- ...ile-web-shell-agent-history-route.test.tsx | 3 +- .../mobile-web-shell-files-route.test.tsx | 3 +- .../mobile-web-shell-session-route.test.tsx | 3 +- .../shell-switch-build-kind.test.tsx | 3 +- .../src/navigation/workspace-route.test.tsx | 38 +++++++++----- mobile/src/navigation/workspace-route.tsx | 15 ++---- mobile/src/transport/use-workspace-client.ts | 2 +- mobile/src/transport/workspace-server.ts | 46 +++++++---------- mobile/src/worktree/relay-hosts-feed.test.ts | 50 ++++++++++++++++--- mobile/src/worktree/relay-hosts-feed.ts | 11 ++-- .../e2e/paired-mobile-desktop-mirror.spec.ts | 22 ++++---- 12 files changed, 121 insertions(+), 88 deletions(-) diff --git a/config/scripts/verify-mobile-web-app-bundle.mjs b/config/scripts/verify-mobile-web-app-bundle.mjs index f9dd0ce7c66..ae07ae2e013 100644 --- a/config/scripts/verify-mobile-web-app-bundle.mjs +++ b/config/scripts/verify-mobile-web-app-bundle.mjs @@ -87,13 +87,12 @@ export const MOBILE_WEB_APP_BUNDLE_ROUTE_SCRIPT_SPREAD = sweptScripts * How far above the measurement the envelope sits, and the only slack a refactor gets. * * Measured, not chosen, and per head rather than cumulative: at 8, 10, 12 and 14 routes the head - * that wrote the old `4r + 16` read 32, 43, 61 and 69, the head that first swept these prefixes - * read 34, 44, 57 and 65, and this one reads 33, 43, 56 and 64. So one head has moved the count by - * as much as four at a fixed route count with no route added (61 to 57), and the step that dropped - * the page's second Zod moved it by one everywhere. Four is that worst step, which is what a shared - * importer set moving between heads costs. Summing the steps instead would grow this number every - * head and loosen the fence for free. A refactor inside four keeps building; anything past it - * re-measures the sweep. + * that wrote the old `4r + 16` read 32, 43, 61 and 69, and one refactor with no route added moved + * the 12-route prefix by four (61 to 57). Four is the worst step a refactor alone has cost. The + * previous sweep read 34, 40, 58 and 71 and this head reads 34, 42, 61 and 77: six at 14 routes, + * from main's own drift plus the server-workspace modules this head adds, so it re-measured the + * sweep rather than fitting inside the margin, which is what a step past four is meant to force. + * Summing the steps instead would grow this number every head and loosen the fence for free. */ export const MOBILE_WEB_APP_BUNDLE_SCRIPT_MARGIN = 4 diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx index 25bc9a7210e..0432bb5d36c 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx @@ -32,8 +32,7 @@ vi.mock('../transport/client-context', () => ({ useHostClient: () => ({ client: null, state: 'disconnected' }) })) vi.mock('../transport/workspace-server', () => ({ - useWorkspaceServer: () => ({ routeHost: undefined, server: null }), - useWorkspaceServerConnState: ({ connState }: { connState: string }) => connState + useWorkspaceServer: () => ({ routeHost: undefined, server: null }) })) vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params })) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx index 9da93eedc30..818f798e189 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx @@ -34,8 +34,7 @@ vi.mock('../transport/client-context', () => ({ useHostClient: () => ({ client: null, state: 'disconnected' }) })) vi.mock('../transport/workspace-server', () => ({ - useWorkspaceServer: () => ({ routeHost: undefined, server: null }), - useWorkspaceServerConnState: ({ connState }: { connState: string }) => connState + useWorkspaceServer: () => ({ routeHost: undefined, server: null }) })) vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params })) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx index b07899745ed..1ebaf6da334 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx @@ -35,8 +35,7 @@ vi.mock('../transport/client-context', () => ({ useHostClient: () => ({ client: null, state: 'disconnected' }) })) vi.mock('../transport/workspace-server', () => ({ - useWorkspaceServer: () => ({ routeHost: undefined, server: null }), - useWorkspaceServerConnState: ({ connState }: { connState: string }) => connState + useWorkspaceServer: () => ({ routeHost: undefined, server: null }) })) vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params, diff --git a/mobile/src/mobile-web-shell/shell-switch-build-kind.test.tsx b/mobile/src/mobile-web-shell/shell-switch-build-kind.test.tsx index 4fea8f3a771..6d7762a3711 100644 --- a/mobile/src/mobile-web-shell/shell-switch-build-kind.test.tsx +++ b/mobile/src/mobile-web-shell/shell-switch-build-kind.test.tsx @@ -42,8 +42,7 @@ vi.mock('../transport/client-context', () => ({ useHostClient: () => ({ client: null, state: 'disconnected' }) })) vi.mock('../transport/workspace-server', () => ({ - useWorkspaceServer: () => ({ routeHost: undefined, server: null }), - useWorkspaceServerConnState: ({ connState }: { connState: string }) => connState + useWorkspaceServer: () => ({ routeHost: undefined, server: null }) })) vi.mock('expo-router', () => ({ Redirect: 'Redirect', diff --git a/mobile/src/navigation/workspace-route.test.tsx b/mobile/src/navigation/workspace-route.test.tsx index 6159ef33f9d..6818f0f5557 100644 --- a/mobile/src/navigation/workspace-route.test.tsx +++ b/mobile/src/navigation/workspace-route.test.tsx @@ -74,8 +74,10 @@ import { HostStatusGatesContext, useOptionalHostProtocolGates } from '../components/host-protocol-gates-context' -import type { ExecutionHostHealth } from '../../../src/shared/execution-host-health' -import { MOBILE_RELAY_HOSTS_LIST_METHOD } from '../../../src/shared/mobile-relay-hosts-contract' +import { + MOBILE_RELAY_HOSTS_LIST_METHOD, + type MobileRelayHost +} from '../../../src/shared/mobile-relay-hosts-contract' import type { HostStatusGates } from '../transport/host-status-gates' import type { ConnectionState } from '../transport/types' import { FakeSession } from '../transport/mobile-endpoint-supervisor-test-fakes' @@ -119,8 +121,15 @@ function desktopGates(hostCapabilities: string[], statusPending = false): HostSt } } -/** The server's health as the desktop reports it in `mobileRelay.hosts.list`. */ -const serverHealth: { current: ExecutionHostHealth } = { current: 'available' } +type ServerReport = Pick +// The pairs the desktop's `mobileRelay.hosts.list` actually sends for a server. +const RUNNING: ServerReport = { health: 'available', relay: 'ready' } +const STOPPED: ServerReport = { health: 'connecting', relay: 'unavailable' } +const NO_STATUS_YET: ServerReport = { health: 'available', relay: 'unavailable' } +const FAILED: ServerReport = { health: 'error', relay: 'unavailable' } + +/** The server as the desktop reports it in `mobileRelay.hosts.list`. */ +const serverReport: { current: ServerReport } = { current: RUNNING } /** A desktop whose own status is settled as given; a status it is asked for is the server's. */ function desktop(serverCapabilities: string[] = []): FakeSession { @@ -133,9 +142,7 @@ function desktop(serverCapabilities: string[] = []): FakeSession { ? { capabilities: serverCapabilities } : method === MOBILE_RELAY_HOSTS_LIST_METHOD ? { - hosts: [ - { hostId: 'runtime:vm', label: 'vm', health: serverHealth.current, relay: 'ready' } - ] + hosts: [{ hostId: 'runtime:vm', label: 'vm', ...serverReport.current }] } : {}, _meta: { runtimeId: 'runtime-1' } @@ -158,7 +165,7 @@ async function render(element: ReactElement, gates: HostStatusGates): Promise { renderer = create(createElement(HostStatusGatesContext.Provider, { value: gates }, element)) @@ -229,24 +236,29 @@ describe('a server workspace', () => { }) }) - it('reads as disconnected while the desktop reports its server down, and recovers', async () => { + it('follows whether the desktop can relay to its server, and recovers', async () => { vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] }) try { route.params = { hostId: 'host-1', executionHost: 'runtime:vm' } desktop() await render(screen, desktopGates(RELAYS)) expect(below.state).toBe('connected') - serverHealth.current = 'connecting' + serverReport.current = STOPPED await act(async () => { vi.advanceTimersByTime(3000) }) expect(below.state).toBe('connecting') - serverHealth.current = 'error' + serverReport.current = NO_STATUS_YET await act(async () => { vi.advanceTimersByTime(3000) }) expect(below.state).toBe('disconnected') - serverHealth.current = 'available' + serverReport.current = FAILED + await act(async () => { + vi.advanceTimersByTime(3000) + }) + expect(below.state).toBe('disconnected') + serverReport.current = RUNNING await act(async () => { vi.advanceTimersByTime(3000) }) @@ -259,7 +271,7 @@ describe('a server workspace', () => { it('opens disconnected, not loading, when its server is already down', async () => { route.params = { hostId: 'host-1', executionHost: 'runtime:vm' } desktop() - serverHealth.current = 'disconnected' + serverReport.current = FAILED await act(async () => { create( createElement(HostStatusGatesContext.Provider, { value: desktopGates(RELAYS) }, screen) diff --git a/mobile/src/navigation/workspace-route.tsx b/mobile/src/navigation/workspace-route.tsx index 2a8265923e6..8453053b932 100644 --- a/mobile/src/navigation/workspace-route.tsx +++ b/mobile/src/navigation/workspace-route.tsx @@ -1,4 +1,4 @@ -import { useMemo, type ReactNode } from 'react' +import type { ReactNode } from 'react' import { useLocalSearchParams } from 'expo-router' import { normalizeExecutionHostId } from '../../../src/shared/execution-host' import { firstParam } from './route-param-reader' @@ -10,7 +10,7 @@ import { import { PageRouteUnavailableScreen } from '../mobile-web-shell/PageRouteUnavailableScreen' import { useHostClient } from '../transport/client-context' import { WorkspaceServerContext } from '../transport/use-workspace-client' -import { useWorkspaceServer, useWorkspaceServerConnState } from '../transport/workspace-server' +import { useWorkspaceServer } from '../transport/workspace-server' const NO_CAPABILITIES: readonly string[] = [] const UNREACHABLE_SERVER_MESSAGE = @@ -37,13 +37,6 @@ export function WorkspaceRoute({ children }: { children: ReactNode }) { desktopCapabilities: desktopGates?.hostCapabilities ?? NO_CAPABILITIES, executionHost }) - const serverConnState = useWorkspaceServerConnState({ client, connState: state, routeHost }) - const serverClient = server ? server.client : undefined - const serverContext = useMemo( - () => - serverClient === undefined ? undefined : { client: serverClient, state: serverConnState }, - [serverClient, serverConnState] - ) // Why: once the desktop has answered, a server it can't reach for this phone is a dead end, not a spinner. if (state === 'connected' && desktopGates && !desktopGates.statusPending && routeHost === null) { return @@ -53,8 +46,8 @@ export function WorkspaceRoute({ children }: { children: ReactNode }) { {children} ) - return server && serverContext ? ( - + return server ? ( + {screens} diff --git a/mobile/src/transport/use-workspace-client.ts b/mobile/src/transport/use-workspace-client.ts index 198982c18ab..b147b1bbb52 100644 --- a/mobile/src/transport/use-workspace-client.ts +++ b/mobile/src/transport/use-workspace-client.ts @@ -12,5 +12,5 @@ export const WorkspaceServerContext = createContext< export function useWorkspaceClient(hostId: string | undefined): ReturnType { const host = useHostClient(hostId) const server = useContext(WorkspaceServerContext) - return server === undefined ? host : { ...host, ...server } + return server === undefined ? host : { ...host, client: server.client, state: server.state } } diff --git a/mobile/src/transport/workspace-server.ts b/mobile/src/transport/workspace-server.ts index 96f499f22b2..e781d96fbc1 100644 --- a/mobile/src/transport/workspace-server.ts +++ b/mobile/src/transport/workspace-server.ts @@ -1,6 +1,6 @@ import type { ExecutionHostId } from '../../../src/shared/execution-host' +import type { MobileRelayHost } from '../../../src/shared/mobile-relay-hosts-contract' import { useRelayHosts } from '../worktree/relay-hosts-feed' -import { serverHostHealth } from '../worktree/server-workspaces' import { scopeRpcClientToExecutionHost, workspaceRouteExecutionHost @@ -21,42 +21,32 @@ export function useWorkspaceServer(args: { executionHost: ExecutionHostId | undefined }): { routeHost: ExecutionHostId | undefined | null - server: { client: RpcClient | null; gates: HostStatusGates } | null + server: { client: RpcClient | null; state: ConnectionState; gates: HostStatusGates } | null } { const { client, connState, desktopCapabilities, executionHost } = args const routeHost = workspaceRouteExecutionHost(client, desktopCapabilities, executionHost) const serverClient = client && routeHost ? scopeRpcClientToExecutionHost(client, routeHost) : null // No hostId: the desktop's recorded version and descriptor must not take the server's. const gates = useHostStatusGates({ hostId: undefined, client: serverClient, connState }) - return { routeHost, server: routeHost === undefined ? null : { client: serverClient, gates } } + // The same feed the host list reads, so the desktop is not polled twice. + const hosts = useRelayHosts(routeHost ? client : null) + const state = + connState === 'connected' && routeHost && hosts + ? serverConnState(hosts.find((entry) => entry.hostId === routeHost)) + : connState + return { + routeHost, + server: routeHost === undefined ? null : { client: serverClient, state, gates } + } } /** - * The connection state a server workspace's screens see: the desktop's, unless the desktop reports - * that server down. Desktop-owned workspaces (`routeHost` absent) keep the desktop's state untouched. + * Whether the desktop can relay calls to the server now. A stopped server reads `connecting`, which + * matches the desktop's own reconnecting banner; one it no longer lists is unreachable. */ -export function useWorkspaceServerConnState(args: { - client: RpcClient | null - connState: ConnectionState - routeHost: ExecutionHostId | undefined | null -}): ConnectionState { - const { client, connState, routeHost } = args - // The same feed the host list reads, so a row's badge and its screens agree. - const hosts = useRelayHosts(routeHost ? client : null) - if (connState !== 'connected' || !routeHost || !hosts) { - return connState - } - const host = hosts.find((entry) => entry.hostId === routeHost) - // A server the desktop no longer lists cannot be reached through it. - switch (host ? serverHostHealth(host) : 'disconnected') { - case 'local': - case 'available': - return 'connected' - case 'connecting': - return 'connecting' - case 'blocked': - case 'disconnected': - case 'error': - return 'disconnected' +function serverConnState(host: MobileRelayHost | undefined): ConnectionState { + if (host?.relay === 'ready') { + return 'connected' } + return host?.health === 'connecting' ? 'connecting' : 'disconnected' } diff --git a/mobile/src/worktree/relay-hosts-feed.test.ts b/mobile/src/worktree/relay-hosts-feed.test.ts index ee5f00d6a96..f190adc1786 100644 --- a/mobile/src/worktree/relay-hosts-feed.test.ts +++ b/mobile/src/worktree/relay-hosts-feed.test.ts @@ -6,14 +6,18 @@ import { MOBILE_RELAY_HOSTS_LIST_METHOD } from '../../../src/shared/mobile-relay import { FakeSession } from '../transport/mobile-endpoint-supervisor-test-fakes' import { relayHostsFeed } from './relay-hosts-feed' +function hostsReply(health: string) { + return { + id: 'reply', + ok: true as const, + result: { hosts: [{ hostId: 'runtime:vm', label: 'vm', health, relay: 'ready' }] }, + _meta: { runtimeId: 'runtime-1' } + } +} + function desktop(health: () => string): FakeSession { const session = new FakeSession('connected') - session.sendRequest.mockImplementation(async () => ({ - id: 'reply', - ok: true, - result: { hosts: [{ hostId: 'runtime:vm', label: 'vm', health: health(), relay: 'ready' }] }, - _meta: { runtimeId: 'runtime-1' } - })) + session.sendRequest.mockImplementation(async () => hostsReply(health())) return session } @@ -61,6 +65,40 @@ describe('the desktop’s server list', () => { expect(listCalls(session)).toBe(1) }) + it('keeps polling after a read slower than the poll', async () => { + const session = desktop(() => 'available') + session.sendRequest.mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 5000)) + return hostsReply('available') + }) + const unsubscribe = relayHostsFeed(session).subscribe(() => {}) + await vi.advanceTimersByTimeAsync(5000) + expect(listCalls(session)).toBe(1) + // The next poll counts from the slow read's finish. + await vi.advanceTimersByTimeAsync(3000) + expect(listCalls(session)).toBe(2) + unsubscribe() + }) + + it('drops a reply that lands after a drop and reconnect', async () => { + const session = desktop(() => 'error') + let releaseStale: () => void = () => {} + session.sendRequest.mockImplementationOnce(async () => { + await new Promise((resolve) => { + releaseStale = resolve + }) + return hostsReply('available') + }) + const feed = relayHostsFeed(session) + const stale = feed.read() + session.publishState('reconnecting') + session.publishState('connected') + expect(await feed.read()).toEqual([expect.objectContaining({ health: 'error' })]) + releaseStale() + expect(await stale).toBeNull() + expect(feed.getSnapshot()?.[0]?.health).toBe('error') + }) + it('forgets its answer on a drop, and reads again on reconnect', async () => { let health = 'available' const session = desktop(() => health) diff --git a/mobile/src/worktree/relay-hosts-feed.ts b/mobile/src/worktree/relay-hosts-feed.ts index e5cfa3cfbe2..4db9bad5d71 100644 --- a/mobile/src/worktree/relay-hosts-feed.ts +++ b/mobile/src/worktree/relay-hosts-feed.ts @@ -1,6 +1,6 @@ // The paired desktop's servers (`mobileRelay.hosts.list`), one feed per desktop client: the host -// list and every open server workspace share one poll and one answer, so a row's badge and its -// workspace screen never disagree and two readers never double the desktop's load. +// list's poll and every open server workspace share one read, so two readers never double the +// desktop's load. The host list reads without subscribing, so its badge may trail a workspace by a tick. import { useCallback, useSyncExternalStore } from 'react' import { AppState } from 'react-native' @@ -8,7 +8,7 @@ import type { MobileRelayHost } from '../../../src/shared/mobile-relay-hosts-con import type { RpcClient } from '../transport/rpc-client' import { relayHostsListRead } from './server-workspace-operations' -// The host list's own poll cadence; a read by any reader pushes the next timed one back. +// The host list's own poll cadence, counted from when the last read finished. const RELAY_HOSTS_POLL_MS = 3000 export type RelayHostsFeed = { @@ -84,14 +84,15 @@ function createFeed(client: RpcClient): RelayHostsFeed { if (inFlight) { return inFlight } - lastReadAt = Date.now() const readGeneration = generation inFlight = fetchHosts(readGeneration).finally(() => { if (readGeneration === generation) { inFlight = null + // Why: scheduling from the finish, so a read slower than the poll still gets a next one. + lastReadAt = Date.now() + schedule() } }) - schedule() return inFlight } diff --git a/tests/e2e/paired-mobile-desktop-mirror.spec.ts b/tests/e2e/paired-mobile-desktop-mirror.spec.ts index 29165752e9a..86a1e5e0e72 100644 --- a/tests/e2e/paired-mobile-desktop-mirror.spec.ts +++ b/tests/e2e/paired-mobile-desktop-mirror.spec.ts @@ -269,13 +269,17 @@ test('phone paired with a desktop lists, opens and types into a workspace on its await host.restartServeProcess({ betweenProcesses: async () => { - // The server is down: the desktop says so, and its workspace stays listed from its last rows. + // The server is down: the desktop reports it reconnecting and not relaying, which the phone + // shows as connecting; its workspace stays listed from its last rows. await expect - .poll(async () => (await listedHost(socket, serverHostId))?.relay, { - timeout: 60_000, - intervals: [1_000] - }) - .toBe('unavailable') + .poll( + async () => { + const down = await listedHost(socket, serverHostId) + return down && { health: down.health, relay: down.relay } + }, + { timeout: 60_000, intervals: [1_000] } + ) + .toEqual({ health: 'connecting', relay: 'unavailable' }) const lastRows = await hostWorktrees(socket, serverHostId) expect(lastRows?.worktrees.map((row) => row.path)).toContain(serverFolder) // A relayed call fails as unavailable, and never runs on the desktop instead. @@ -294,13 +298,13 @@ test('phone paired with a desktop lists, opens and types into a workspace on its } } }) - // Back up: the desktop reports it available again, and the same phone socket reaches it. + // Back up: the desktop relays to it again, and the same phone socket reaches it. await expect - .poll(async () => (await listedHost(socket, serverHostId))?.health, { + .poll(async () => (await listedHost(socket, serverHostId))?.relay, { timeout: 90_000, intervals: [2_000] }) - .toBe('available') + .toBe('ready') const reopened = await call( socket, { id: 'ps-restarted', ...list, executionHost: serverHostId },