diff --git a/mobile/src/diagnostics/use-mobile-web-bundle-probe.test.tsx b/mobile/src/diagnostics/use-mobile-web-bundle-probe.test.tsx index a9a71bc4309..e339d6125de 100644 --- a/mobile/src/diagnostics/use-mobile-web-bundle-probe.test.tsx +++ b/mobile/src/diagnostics/use-mobile-web-bundle-probe.test.tsx @@ -1,6 +1,6 @@ import { createElement } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { RpcClient } from '../transport/rpc-client' import type { MobileWebBundleFetchResult } from '../transport/mobile-web-bundle-fetch' @@ -117,6 +117,30 @@ function fetchedBundle(): MobileWebBundleFetchResult { } } +type Deferred = { + readonly promise: Promise + resolve: (value: T) => void + reject: (error: unknown) => void +} + +function deferred(): Deferred { + const box: { resolve: (value: T) => void; reject: (error: unknown) => void } = { + resolve: () => {}, + reject: () => {} + } + const promise = new Promise((resolve, reject) => { + box.resolve = resolve + box.reject = reject + }) + return { promise, resolve: box.resolve, reject: box.reject } +} + +async function settle(): Promise { + await act(async () => { + await Promise.resolve() + }) +} + beforeEach(() => { push.attach.mockReset().mockReturnValue(push.detach) push.detach.mockReset() @@ -125,6 +149,10 @@ beforeEach(() => { fetchMock.mockReset() }) +afterEach(() => { + vi.useRealTimers() +}) + describe('useMobileWebBundleProbe', () => { it('dials no host until the row is tapped', async () => { const probe = await renderProbe(HOST.id) @@ -181,6 +209,85 @@ describe('useMobileWebBundleProbe', () => { expect(probe.state).toEqual({ status: 'failed', detail: 'no paired host to fetch from' }) }) + it('gives up on a host whose client never arrives instead of waiting forever', async () => { + vi.useFakeTimers() + // The host is not in the store, so no client is ever acquired for it and `awaitingHost` would + // otherwise stay true with the row's button disabled for the life of the screen. + loadHostsMock.mockResolvedValue([]) + const probe = await renderProbe(HOST.id) + + await probe.run() + expect(probe.state).toEqual({ status: 'running' }) + expect(probe.awaitingHost).toBe(true) + + await act(async () => { + await vi.advanceTimersByTimeAsync(9_999) + }) + expect(probe.state).toEqual({ status: 'running' }) + + await act(async () => { + await vi.advanceTimersByTimeAsync(1) + }) + expect(probe.state).toEqual({ status: 'failed', detail: 'no client for the host within 10s' }) + // The row re-enables its button off `running`, and nothing is left dialling the host. + expect(probe.awaitingHost).toBe(false) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('does not clear the deadline for a host that did arrive', async () => { + vi.useFakeTimers() + fetchMock.mockReturnValue(new Promise(() => {})) + const probe = await renderProbe(HOST.id) + + await probe.run() + expect(fetchMock).toHaveBeenCalledTimes(1) + + await act(async () => { + await vi.advanceTimersByTimeAsync(60_000) + }) + + // A slow fetch is not a host that never opened: the deadline covers acquiring the client only. + expect(probe.state).toEqual({ status: 'running' }) + }) + + it('ignores a result from a run the screen already moved on from', async () => { + const first = deferred() + const second = deferred() + fetchMock.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) + const probe = await renderProbe(HOST.id) + + await probe.run() + await probe.run() + first.resolve({ ...fetchedBundle(), elapsedMs: 999 }) + await settle() + + expect(probe.state).toEqual({ status: 'running' }) + + second.resolve(fetchedBundle()) + await settle() + + expect(probe.state).toMatchObject({ status: 'done', elapsedMs: 12 }) + }) + + it('ignores a failure from a run the screen already moved on from', async () => { + const first = deferred() + const second = deferred() + fetchMock.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) + const probe = await renderProbe(HOST.id) + + await probe.run() + await probe.run() + first.reject(new Error('invalid_argument: mobile_web_bundle_unavailable')) + await settle() + + expect(probe.state).toEqual({ status: 'running' }) + + second.resolve(fetchedBundle()) + await settle() + + expect(probe.state).toMatchObject({ status: 'done', elapsedMs: 12 }) + }) + it('aborts the run it started when the screen goes away', async () => { const captured: { signal: AbortSignal | null } = { signal: null } fetchMock.mockImplementation((args: { signal?: AbortSignal }) => { diff --git a/mobile/src/diagnostics/use-mobile-web-bundle-probe.ts b/mobile/src/diagnostics/use-mobile-web-bundle-probe.ts index 9f780e06571..aeebb4573a5 100644 --- a/mobile/src/diagnostics/use-mobile-web-bundle-probe.ts +++ b/mobile/src/diagnostics/use-mobile-web-bundle-probe.ts @@ -2,6 +2,17 @@ import { useCallback, useEffect, useState } from 'react' import { useHostClient } from '../transport/client-context' import { fetchMobileWebBundle } from '../transport/mobile-web-bundle-fetch' import { readMobileWebBundleErrorCode } from '../transport/mobile-web-bundle-operations' +import { startDiagnosticFetchTimeout } from './diagnostic-fetch-timeout' + +/** + * How long a tap waits for the host's client object before it gives up. + * + * Generous, because acquiring one can queue behind another screen's, but bounded, because none of + * that is a network round trip: the connect and request timeouts live below this, inside the fetch, + * and only apply once a client exists. Without a bound here a host that never opens leaves the row + * reading `Connecting…` with its button disabled for the life of the screen. + */ +const HOST_CLIENT_DIAL_DEADLINE_MS = 10_000 export type MobileWebBundleProbeState = | { status: 'idle' } @@ -76,6 +87,27 @@ export function useMobileWebBundleProbe(hostId: string | null): { } }, [client, request]) + useEffect(() => { + if (request === null || client !== null) { + return + } + const deadline = startDiagnosticFetchTimeout(HOST_CLIENT_DIAL_DEADLINE_MS) + const giveUp = () => { + setState({ + status: 'failed', + detail: `no client for the host within ${HOST_CLIENT_DIAL_DEADLINE_MS / 1000}s` + }) + // Drops the acquisition too, so a host that never opens stops being dialled. + setRequest(null) + } + deadline.signal.addEventListener('abort', giveUp) + return () => { + // Removed first: `dispose` aborts a signal it has not already aborted. + deadline.signal.removeEventListener('abort', giveUp) + deadline.dispose() + } + }, [client, request]) + const run = useCallback(() => { if (hostId === null) { setState({ status: 'failed', detail: 'no paired host to fetch from' }) diff --git a/mobile/src/test-support/rpc-recording/mutants/operation-mutations.ts b/mobile/src/test-support/rpc-recording/mutants/operation-mutations.ts index d4470c476da..58b12257cdf 100644 --- a/mobile/src/test-support/rpc-recording/mutants/operation-mutations.ts +++ b/mobile/src/test-support/rpc-recording/mutants/operation-mutations.ts @@ -73,10 +73,6 @@ export const OPERATION_MUTATIONS = { before: 'const snapshot = decodeAccountsSnapshot(accounts.value)', after: 'const snapshot = decodeAccountsSnapshot(reply)' }, - // Puts the workspace catalog's reply back behind an unchecked reader, so a reply carrying neither - // rows nor an `unchanged` token reaches `admitWorktreeCatalogResponse` as an invalid admission - // instead of being named at the boundary — main's answer, and the one the host screen showed as - // an empty host rather than a failure (STA-3123). // Writes every chunk of an asset at offset 0, so a multi-chunk asset reassembles as its last // chunk over a zero-filled buffer. The length still matches the manifest; only the sha256 check // and the decoded bytes in the projection say the bundle is wrong. @@ -85,6 +81,10 @@ export const OPERATION_MUTATIONS = { before: 'whole.set(bytes, offset)', after: 'whole.set(bytes, 0)' }, + // Puts the workspace catalog's reply back behind an unchecked reader, so a reply carrying neither + // rows nor an `unchanged` token reaches `admitWorktreeCatalogResponse` as an invalid admission + // instead of being named at the boundary — main's answer, and the one the host screen showed as + // an empty host rather than a failure (STA-3123). 'worktree-catalog-unchecked-reader': { file: 'worktree-catalog-operations.ts', before: "read: rpcResultVariant('worktree-catalog', worktreeCatalogSchema)", diff --git a/mobile/src/transport/mobile-web-bundle-fetch.test.ts b/mobile/src/transport/mobile-web-bundle-fetch.test.ts index 00a72937643..70856553407 100644 --- a/mobile/src/transport/mobile-web-bundle-fetch.test.ts +++ b/mobile/src/transport/mobile-web-bundle-fetch.test.ts @@ -241,6 +241,51 @@ describe('fetchMobileWebBundle', () => { ) }) + it('refuses a chunk that answers the right path at the wrong offset', async () => { + // The path half of the echo check is already covered; this is the offset half on its own, so + // a host that re-serves chunk zero cannot have its bytes written at the offset we asked for. + const host = bundleHost( + { 'index.html': 'abcdef' }, + { + chunkBytes: 3, + intercept: (call) => + call.method === 'mobileWeb.bundle.chunk' && paramField(call.params, 'offset') === 3 + ? { + buildId: BUILD_ID, + path: 'index.html', + offset: 0, + assetByteLength: 6, + sha256: toHex(sha256(bytesOf('abcdef'))), + dataBase64: encodeBase64(bytesOf('abc')), + eof: false + } + : undefined + } + ) + + await expect(fetchMobileWebBundle({ client: host.client })).rejects.toThrow( + 'bundle chunk answered index.html at 0, not index.html at 3' + ) + }) + + it('reads a zero-byte asset in one chunk and returns it empty', async () => { + // A real bundle carries these. The asset is whole the moment the host says eof, and nothing + // else in the loop can end it: a zero-length reply is otherwise how a host makes no progress. + const host = bundleHost({ 'assets/empty.css': '', 'index.html': 'abc' }) + + const fetched = await fetchMobileWebBundle({ client: host.client }) + + expect(fetched.assets.get('assets/empty.css')).toEqual(new Uint8Array(0)) + expect(fetched.totalBytes).toBe(3) + expect( + host.calls.filter( + (call) => + call.method === 'mobileWeb.bundle.chunk' && + paramField(call.params, 'path') === 'assets/empty.css' + ) + ).toHaveLength(1) + }) + it('never puts a fifth chunk request on one connection', async () => { const peaks: number[] = [] const host = bundleHost( diff --git a/mobile/src/transport/mobile-web-bundle-fetch.ts b/mobile/src/transport/mobile-web-bundle-fetch.ts index 87f34ab70ee..2823085437a 100644 --- a/mobile/src/transport/mobile-web-bundle-fetch.ts +++ b/mobile/src/transport/mobile-web-bundle-fetch.ts @@ -53,7 +53,6 @@ export async function fetchMobileWebBundle(args: { const worker = async (): Promise => { try { for (let asset = pending.shift(); asset !== undefined; asset = pending.shift()) { - throwIfStopped(args.signal, stopped.signal) const bytes = await readBundleAsset({ client: args.client, asset, @@ -92,6 +91,9 @@ async function readBundleAsset(args: { signal?: AbortSignal stopped: AbortSignal }): Promise { + // Before the buffer, not after: an asset can be a tenth of the total ceiling, and a worker that + // picked one up after a sibling failed would otherwise allocate it only to drop it. + throwIfStopped(args.signal, args.stopped) const whole = new Uint8Array(args.asset.byteLength) let offset = 0 for (;;) { diff --git a/mobile/src/transport/mobile-web-bundle-reply-schemas.test.ts b/mobile/src/transport/mobile-web-bundle-reply-schemas.test.ts index 012c2e48f56..3b80906d836 100644 --- a/mobile/src/transport/mobile-web-bundle-reply-schemas.test.ts +++ b/mobile/src/transport/mobile-web-bundle-reply-schemas.test.ts @@ -98,6 +98,57 @@ describe('mobile web bundle manifest reply reader', () => { } }) + it('bounds what a manifest can make the fetch allocate, however it declares totalBytes', () => { + // The ceilings above bound each asset and the asset count, and `totalBytes` separately. None + // of them bounds the product, which is what the fetch allocates. + const oversized = Array.from({ length: MOBILE_WEB_BUNDLE_MAX_ASSETS }, (_, index) => + asset({ path: `assets/${index}.js`, byteLength: MOBILE_WEB_BUNDLE_MAX_ASSET_BYTES }) + ) + expect(readManifest(manifestReply({ totalBytes: 0, assets: oversized })).compatible).toBe(false) + expect(readManifest(manifestReply({ totalBytes: 12, assets: oversized })).compatible).toBe( + false + ) + }) + + it('accepts a bundle that sums to the ceiling and refuses one byte more', () => { + // Four assets, because one quarter of the total ceiling is the largest share that still fits + // under the per-asset ceiling. `lastByteLength` moves only the final one. + const quarter = MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES / 4 + const spread = (lastByteLength: number) => + Array.from({ length: 4 }, (_, index) => + asset({ path: `assets/${index}.js`, byteLength: index === 3 ? lastByteLength : quarter }) + ) + expect( + readManifest( + manifestReply({ totalBytes: MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES, assets: spread(quarter) }) + ).compatible + ).toBe(true) + expect( + readManifest( + manifestReply({ + totalBytes: MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES, + assets: spread(quarter + 1) + }) + ).compatible + ).toBe(false) + }) + + it('refuses one asset over the per-asset ceiling and accepts one at it', () => { + expect( + readManifest( + manifestReply({ + totalBytes: MOBILE_WEB_BUNDLE_MAX_ASSET_BYTES, + assets: [asset({ byteLength: MOBILE_WEB_BUNDLE_MAX_ASSET_BYTES })] + }) + ).compatible + ).toBe(true) + expect( + readManifest( + manifestReply({ assets: [asset({ byteLength: MOBILE_WEB_BUNDLE_MAX_ASSET_BYTES + 1 })] }) + ).compatible + ).toBe(false) + }) + it('refuses a schemaVersion it does not know rather than guessing at the shape', () => { expect(readManifest(manifestReply({ schemaVersion: 2 })).compatible).toBe(false) expect(readManifest(manifestReply({ schemaVersion: undefined })).compatible).toBe(false) @@ -221,6 +272,11 @@ describe('mobile web bundle error codes', () => { expect( readMobileWebBundleErrorCode(new Error('RPC mobile_web_bundle_unavailable failed')) ).toBeNull() + // The second position is `: `, exactly. Slicing the leading token's length off + // any message would make this one read as the code that follows the bracket. + expect( + readMobileWebBundleErrorCode(new Error('rpc (mobile_web_bundle_unavailable)')) + ).toBeNull() }) it('reads a dispatcher schema refusal, whose message is prose, as no code at all', () => { diff --git a/mobile/src/transport/mobile-web-bundle-reply-schemas.ts b/mobile/src/transport/mobile-web-bundle-reply-schemas.ts index 4a3445a5a5f..aa1ba5ec662 100644 --- a/mobile/src/transport/mobile-web-bundle-reply-schemas.ts +++ b/mobile/src/transport/mobile-web-bundle-reply-schemas.ts @@ -37,13 +37,25 @@ const assetSchema = z.looseObject({ * * `schemaVersion` stays a literal because the manifest is closed in both directions: a bump is the * only change path, and an unrecognised one is an unusable bundle to re-fetch, never a crash. */ -const manifestSchema = z.looseObject({ - schemaVersion: z.literal(MOBILE_WEB_BUNDLE_SCHEMA_VERSION), - buildId: z.string().regex(SHA256_PATTERN), - entrypoint: MobileWebBundleAssetPathSchema, - totalBytes: z.number().int().nonnegative().max(MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES), - assets: z.array(assetSchema).min(1).max(MOBILE_WEB_BUNDLE_MAX_ASSETS) -}) +const manifestSchema = z + .looseObject({ + schemaVersion: z.literal(MOBILE_WEB_BUNDLE_SCHEMA_VERSION), + buildId: z.string().regex(SHA256_PATTERN), + entrypoint: MobileWebBundleAssetPathSchema, + totalBytes: z.number().int().nonnegative().max(MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES), + assets: z.array(assetSchema).min(1).max(MOBILE_WEB_BUNDLE_MAX_ASSETS) + }) + // The allocation bound, and the reason it is the sum rather than `totalBytes`: the fetch + // allocates one buffer per asset from `byteLength` and holds them all, so a manifest declaring + // `totalBytes` 0 alongside 256 assets of 10 MiB each would pass every ceiling above and still + // cost 2560 MiB. The host pins sum === totalBytes; this client never trusts `totalBytes` for + // anything, so it bounds what it will actually allocate instead. + .refine( + (manifest) => + manifest.assets.reduce((sum, asset) => sum + asset.byteLength, 0) <= + MOBILE_WEB_BUNDLE_MAX_TOTAL_BYTES, + 'assets sum to more than the contract total' + ) /** `chunkBytes` is read, never assumed: the host may shrink it without a client release. Capped at * the constant because a larger value would overshoot `dataBase64` above. */ @@ -66,6 +78,5 @@ export const MobileWebBundleChunkReplySchema = z.looseObject({ }) export type MobileWebBundleManifestReply = z.output -export type MobileWebBundleChunkReply = z.output export type MobileWebBundleManifestRead = MobileWebBundleManifestReply['manifest'] export type MobileWebBundleAssetRead = MobileWebBundleManifestRead['assets'][number] diff --git a/mobile/src/transport/rpc-operation.ts b/mobile/src/transport/rpc-operation.ts index 620f1516a12..93424f35433 100644 --- a/mobile/src/transport/rpc-operation.ts +++ b/mobile/src/transport/rpc-operation.ts @@ -175,7 +175,8 @@ export async function runRpcOperation< operation: RpcOperation, // Shares the deferred sender's tuple so the two cannot disagree about what a params-less // method may be called with: the catalog types those `void`, and an explicit `null` is the - // frame several shipped senders already put on the wire. + // frame three of them go out with today (`notifications.testPush`, + // `notifications.unregisterPush`, `speech.models.list`), all through the deferred entry point. ...args: RpcSendArguments ): Promise> { const outcome = await request(client, operation, args[0], args[1])