diff --git a/src/daemon/handlers/__tests__/record-trace.test.ts b/src/daemon/handlers/__tests__/record-trace.test.ts index ed5e97183..977bcb005 100644 --- a/src/daemon/handlers/__tests__/record-trace.test.ts +++ b/src/daemon/handlers/__tests__/record-trace.test.ts @@ -64,6 +64,7 @@ import { overlayRecordingTouches, } from '../../../recording/overlay.ts'; import { runCmd, runCmdBackground } from '../../../utils/exec.ts'; +import { isPlayableVideo, waitForStableFile } from '../../../utils/video.ts'; type RunnerCall = { command: string; @@ -81,6 +82,8 @@ const mockRunIosRunnerCommand = vi.mocked(runIosRunnerCommand); const mockResizeRecording = vi.mocked(resizeRecording); const mockTrimRecordingStart = vi.mocked(trimRecordingStart); const mockOverlayRecordingTouches = vi.mocked(overlayRecordingTouches); +const mockWaitForStableFile = vi.mocked(waitForStableFile); +const mockIsPlayableVideo = vi.mocked(isPlayableVideo); const overlaySupportWarning = getRecordingOverlaySupportWarning(); @@ -183,6 +186,8 @@ beforeEach(() => { mockResizeRecording.mockImplementation(async () => {}); mockTrimRecordingStart.mockImplementation(async () => {}); mockOverlayRecordingTouches.mockImplementation(async () => {}); + mockWaitForStableFile.mockImplementation(async () => {}); + mockIsPlayableVideo.mockImplementation(async () => true); }); afterEach(() => { @@ -596,6 +601,138 @@ test('record stop leaves a short visual tail after iOS simulator gestures', asyn expect(kill).toHaveBeenCalledWith('SIGINT'); }); +test('record stop reports too-short iOS simulator recordings without leaving invalid output', async () => { + const sessionStore = makeSessionStore(); + const sessionName = 'ios-sim-too-short'; + const outPath = path.join(os.tmpdir(), `agent-device-too-short-${Date.now()}.mp4`); + fs.writeFileSync(outPath, 'not-a-video'); + const session = makeSession(sessionName, { + platform: 'ios', + id: 'sim-1', + name: 'Simulator', + kind: 'simulator', + booted: true, + }); + session.recording = { + platform: 'ios', + outPath, + startedAt: Date.now(), + showTouches: false, + gestureEvents: [], + child: { kill: vi.fn() }, + wait: Promise.resolve({ stdout: '', stderr: 'failed to finalize', exitCode: 1 }), + }; + sessionStore.set(sessionName, session); + + const response = await runRecordCommand({ + sessionStore, + sessionName, + positionals: ['stop'], + }); + + expect(response?.ok).toBe(false); + expect((response as any).error?.message).toMatch(/wait at least 1000ms/i); + expect((response as any).error?.message).toMatch(/failed to finalize/i); + expect(fs.existsSync(outPath)).toBe(false); +}); + +test('record stop measures too-short iOS simulator recordings from stop request time', async () => { + vi.useFakeTimers(); + vi.setSystemTime(10_000); + const sessionStore = makeSessionStore(); + const sessionName = 'ios-sim-too-short-delayed-finalize'; + const outPath = path.join(os.tmpdir(), `agent-device-too-short-delayed-${Date.now()}.mp4`); + fs.writeFileSync(outPath, 'not-a-video'); + const session = makeSession(sessionName, { + platform: 'ios', + id: 'sim-1', + name: 'Simulator', + kind: 'simulator', + booted: true, + }); + session.recording = { + platform: 'ios', + outPath, + startedAt: Date.now() - 500, + showTouches: false, + gestureEvents: [], + child: { kill: vi.fn() }, + wait: Promise.resolve({ stdout: '', stderr: '', exitCode: 0 }), + }; + sessionStore.set(sessionName, session); + mockWaitForStableFile.mockImplementation(async () => { + vi.setSystemTime(11_300); + }); + mockIsPlayableVideo.mockImplementation(async () => false); + + const response = await runRecordCommand({ + sessionStore, + sessionName, + positionals: ['stop'], + }); + + expect(response?.ok).toBe(false); + expect((response as any).error?.message).toMatch(/Recording stopped after 500ms/i); + expect((response as any).error?.message).toMatch(/wait at least 1000ms/i); + expect(fs.existsSync(outPath)).toBe(false); +}); + +test('record stop measures too-short Android failures from stop request time', async () => { + vi.useFakeTimers(); + vi.setSystemTime(20_000); + const sessionStore = makeSessionStore(); + const sessionName = 'android-too-short-delayed-stop'; + const session = makeSession(sessionName, { + platform: 'android', + id: 'emulator-5554', + name: 'Android', + kind: 'device', + booted: true, + }); + session.recording = { + platform: 'android', + outPath: path.resolve('./android-too-short.mp4'), + startedAt: Date.now() - 500, + showTouches: true, + gestureEvents: [], + remotePath: '/sdcard/agent-device-recording-too-short.mp4', + remotePid: '4322', + chunks: [ + { + index: 1, + path: path.resolve('./android-too-short.mp4'), + remotePath: '/sdcard/agent-device-recording-too-short.mp4', + }, + ], + }; + sessionStore.set(sessionName, session); + mockRunCmd.mockImplementation(async (_cmd, args) => { + const command = args.join(' '); + if (command === '-s emulator-5554 shell ps -o pid= -p 4322') { + return { stdout: '4322\n', stderr: '', exitCode: 0 }; + } + if (command === '-s emulator-5554 shell kill -2 4322') { + vi.setSystemTime(21_300); + return { stdout: '', stderr: 'failed to stop', exitCode: 1 }; + } + if (command === '-s emulator-5554 shell kill -9 4322') { + return { stdout: '', stderr: 'failed to force stop', exitCode: 1 }; + } + return { stdout: '', stderr: '', exitCode: 0 }; + }); + + const response = await runRecordCommand({ + sessionStore, + sessionName, + positionals: ['stop'], + }); + + expect(response?.ok).toBe(false); + expect((response as any).error?.message).toMatch(/Recording stopped after 500ms/i); + expect((response as any).error?.message).toMatch(/wait at least 1000ms/i); + expect((response as any).error?.message).toMatch(/failed to stop/i); +}); + test('record start stores iOS simulator recorder pid for scoped cleanup', async () => { const sessionStore = makeSessionStore(); const sessionName = 'ios-sim-recorder-pid'; diff --git a/src/daemon/handlers/record-trace-android-copy.ts b/src/daemon/handlers/record-trace-android-copy.ts index 2ea4e3178..a12f7922f 100644 --- a/src/daemon/handlers/record-trace-android-copy.ts +++ b/src/daemon/handlers/record-trace-android-copy.ts @@ -43,11 +43,7 @@ async function copyAndroidRecordingWithValidation(params: { let lastCopyError: string | undefined; for (let attempt = 0; attempt < ANDROID_LOCAL_VIDEO_ATTEMPTS; attempt += 1) { - try { - fs.rmSync(outPath, { force: true }); - } catch { - // Ignore stale local file cleanup issues and let adb pull report the real failure. - } + removeLocalRecordingCandidate(outPath); const device = androidDeviceForSerial(deviceId); const pullResult = await pullAndroidAdbFile(remotePath, outPath, { @@ -98,6 +94,7 @@ async function copyAndroidRecordingWithValidation(params: { if (lastCopyError) { return `failed to copy recording from device: ${lastCopyError}`; } + removeLocalRecordingCandidate(outPath); return 'failed to copy recording from device: pulled file is not a playable MP4'; } @@ -108,3 +105,11 @@ function readFileSize(filePath: string): number { return 0; } } + +function removeLocalRecordingCandidate(filePath: string): void { + try { + fs.rmSync(filePath, { force: true }); + } catch { + // Ignore local cleanup issues and let the caller report the validation failure. + } +} diff --git a/src/daemon/handlers/record-trace-android.ts b/src/daemon/handlers/record-trace-android.ts index 138e01ff7..ba5e1decb 100644 --- a/src/daemon/handlers/record-trace-android.ts +++ b/src/daemon/handlers/record-trace-android.ts @@ -2,7 +2,7 @@ import { emitDiagnostic } from '../../utils/diagnostics.ts'; import { sleep } from '../../utils/timeouts.ts'; import { androidDeviceForSerial, runAndroidAdb } from '../../platforms/android/adb.ts'; import type { DaemonResponse, SessionState } from '../types.ts'; -import { formatRecordTraceExecFailure } from '../record-trace-errors.ts'; +import { buildRecordStopFailure, formatRecordTraceExecFailure } from '../record-trace-errors.ts'; import type { RecordTraceDeps } from './record-trace-types.ts'; import { errorResponse } from './response.ts'; import type { @@ -413,8 +413,9 @@ export async function stopAndroidRecording(params: { deps: RecordTraceDeps; device: AndroidDevice; recording: AndroidRecording; + stopRequestedAt: number; }): Promise { - const { deps, device, recording } = params; + const { deps, device, recording, stopRequestedAt } = params; emitDiagnostic({ level: 'debug', phase: 'record_stop_android_enter', @@ -444,7 +445,10 @@ export async function stopAndroidRecording(params: { }); if (copyError) { await cleanupRemoteRecording(); - return errorResponse('COMMAND_FAILED', copyError); + return errorResponse( + 'COMMAND_FAILED', + formatAndroidStopFailure(copyError, recording, stopRequestedAt), + ); } await finalizeAndroidRecordingOutput({ recording, deps }); @@ -453,7 +457,10 @@ export async function stopAndroidRecording(params: { await cleanupRemoteRecording(); if (stopError) { - return errorResponse('COMMAND_FAILED', stopError); + return errorResponse( + 'COMMAND_FAILED', + formatAndroidStopFailure(stopError, recording, stopRequestedAt), + ); } if (cleanupError) { @@ -488,3 +495,11 @@ export async function stopAndroidRecording(params: { } } } + +function formatAndroidStopFailure( + error: string, + recording: AndroidRecording, + stopRequestedAt: number, +): string { + return buildRecordStopFailure(error, recording, stopRequestedAt).message; +} diff --git a/src/daemon/handlers/record-trace-recording.ts b/src/daemon/handlers/record-trace-recording.ts index 8ba9e48ef..d7c3700c8 100644 --- a/src/daemon/handlers/record-trace-recording.ts +++ b/src/daemon/handlers/record-trace-recording.ts @@ -16,7 +16,11 @@ import { resizeRecording, trimRecordingStart, } from '../../recording/overlay.ts'; -import { formatRecordTraceError, formatRecordTraceExecFailure } from '../record-trace-errors.ts'; +import { + buildRecordStopFailure, + formatRecordTraceError, + formatRecordTraceExecFailure, +} from '../record-trace-errors.ts'; import { resolveRecordingProvider } from '../recording-provider.ts'; import { finalizeRecordingOverlay } from './record-trace-finalize.ts'; import { errorResponse } from './response.ts'; @@ -304,10 +308,11 @@ async function stopNonRunnerRecording(params: { deps: RecordTraceDeps; device: SessionState['device']; recording: Extract, { platform: 'ios' | 'android' }>; + stopRequestedAt: number; }): Promise { - const { deps, device, recording } = params; + const { deps, device, recording, stopRequestedAt } = params; if (recording.platform === 'android') { - return await stopAndroidRecording({ deps, device, recording }); + return await stopAndroidRecording({ deps, device, recording, stopRequestedAt }); } await withDiagnosticTimer('record_stop_tail_settle', () => deps.waitForRecordingTail(recording), { @@ -322,15 +327,17 @@ async function stopNonRunnerRecording(params: { }, ); if (!stopResult) { - return errorResponse( - 'COMMAND_FAILED', + return buildIosSimulatorRecordingStopFailure( `failed to stop recording: simctl recordVideo did not exit after ${IOS_SIMULATOR_RECORDING_STOP_TIMEOUT_MS}ms and forced cleanup`, + recording, + stopRequestedAt, ); } if (stopResult.exitCode !== 0) { - return errorResponse( - 'COMMAND_FAILED', + return buildIosSimulatorRecordingStopFailure( `failed to stop recording: ${formatRecordTraceExecFailure(stopResult, 'simctl recordVideo')}`, + recording, + stopRequestedAt, ); } @@ -353,9 +360,10 @@ async function stopNonRunnerRecording(params: { }, ); if (!playable) { - return errorResponse( - 'COMMAND_FAILED', + return buildIosSimulatorRecordingStopFailure( `failed to stop recording: ${recording.outPath} was not finalized into a playable video`, + recording, + stopRequestedAt, ); } @@ -398,6 +406,24 @@ async function stopNonRunnerRecording(params: { return null; } +function buildIosSimulatorRecordingStopFailure( + message: string, + recording: Extract, { platform: 'ios' }>, + stopRequestedAt: number, +): DaemonResponse { + const failure = buildRecordStopFailure(message, recording, stopRequestedAt); + removeInvalidRecordingOutput(recording.outPath); + return errorResponse('COMMAND_FAILED', failure.message); +} + +function removeInvalidRecordingOutput(outPath: string): void { + try { + fs.rmSync(outPath, { force: true }); + } catch { + // Best effort: the error response still reports the failed finalization. + } +} + async function stopRecording(params: { req: DaemonRequest; activeSession: SessionState; @@ -412,6 +438,7 @@ async function stopRecording(params: { } const recording = activeSession.recording; + const stopRequestedAt = Date.now(); const invalidatedReason = recording.invalidatedReason; activeSession.recording = undefined; @@ -420,7 +447,7 @@ async function stopRecording(params: { ? await stopIosDeviceRecording({ req, activeSession, device, logPath, deps, recording }) : recording.platform === 'macos-runner' ? await stopMacOsRecording({ req, activeSession, device, logPath, deps, recording }) - : await stopNonRunnerRecording({ deps, device, recording }); + : await stopNonRunnerRecording({ deps, device, recording, stopRequestedAt }); if (stopError) { return stopError; } diff --git a/src/daemon/record-trace-errors.ts b/src/daemon/record-trace-errors.ts index 4cc0509c5..40a17ea33 100644 --- a/src/daemon/record-trace-errors.ts +++ b/src/daemon/record-trace-errors.ts @@ -2,6 +2,17 @@ export function formatRecordTraceError(error: unknown): string { return error instanceof Error ? error.message : String(error); } +const MIN_PLAYABLE_RECORDING_DURATION_MS = 1_000; + +type RecordingStartedAt = { + startedAt: number; +}; + +type RecordStopFailure = { + message: string; + tooShort: boolean; +}; + export function formatRecordTraceExecFailure( result: { stdout: string; stderr: string; exitCode: number }, command: string, @@ -10,3 +21,18 @@ export function formatRecordTraceExecFailure( result.stderr.trim() || result.stdout.trim() || `${command} exited with code ${result.exitCode}` ); } + +export function buildRecordStopFailure( + message: string, + recording: RecordingStartedAt, + now = Date.now(), +): RecordStopFailure { + const elapsedMs = Math.max(0, now - recording.startedAt); + if (elapsedMs >= MIN_PLAYABLE_RECORDING_DURATION_MS) { + return { message, tooShort: false }; + } + return { + message: `${message}. Recording stopped after ${Math.round(elapsedMs)}ms; wait at least ${MIN_PLAYABLE_RECORDING_DURATION_MS}ms between record start and record stop so the recorder can finalize a playable MP4`, + tooShort: true, + }; +} diff --git a/src/daemon/snapshot-presentation/ios/noise.ts b/src/daemon/snapshot-presentation/ios/noise.ts index d20a8b7d5..89b991c44 100644 --- a/src/daemon/snapshot-presentation/ios/noise.ts +++ b/src/daemon/snapshot-presentation/ios/noise.ts @@ -4,8 +4,9 @@ import { normalizeType } from '../../snapshot-processing.ts'; import { collectIosScrollIndicatorPresentation } from './scroll.ts'; import { areRectsApproximatelyEqual, - collectDescendants, + findDescendant, findLargestViewportRect, + forEachDescendant, isRepeatedStaticNode, isScrollableSnapshotType, isSemanticActionNode, @@ -35,9 +36,13 @@ function collectIosReactNativeOverlayWrapperSuppression( return; } - const hasVisibleBannerDescendant = collectDescendants(nodes, position).some( - (descendant) => - descendant.label?.trim() === nodeLabel && isReactNativeCollapsedWarningBanner(descendant), + const hasVisibleBannerDescendant = Boolean( + findDescendant( + nodes, + position, + (descendant) => + descendant.label?.trim() === nodeLabel && isReactNativeCollapsedWarningBanner(descendant), + ), ); if (hasVisibleBannerDescendant) { suppressedIndexes.add(node.index); @@ -81,45 +86,48 @@ function collectRepeatedStaticSuppressionForNode( nodeLabel: string, suppressedIndexes: Set, ): void { - const descendants = collectDescendants(nodes, position); const type = normalizeType(node.type ?? ''); if (type === 'statictext' || type === 'link') { - suppressRepeatedStaticDescendants(descendants, nodeLabel, suppressedIndexes); + suppressRepeatedStaticDescendants(nodes, position, nodeLabel, suppressedIndexes); return; } if (type !== 'other') { return; } - if (hasEquivalentSemanticDescendant(descendants, nodeLabel)) { + if (hasEquivalentSemanticDescendant(nodes, position, nodeLabel)) { suppressedIndexes.add(node.index); return; } - suppressRepeatedStaticDescendants(descendants, nodeLabel, suppressedIndexes); + suppressRepeatedStaticDescendants(nodes, position, nodeLabel, suppressedIndexes); } function hasEquivalentSemanticDescendant( - descendants: RawSnapshotNode[], + nodes: RawSnapshotNode[], + position: number, nodeLabel: string, ): boolean { - return descendants.some((descendant) => { - const type = normalizeType(descendant.type ?? ''); - return ( - (type === 'link' || type === 'searchfield' || isScrollableSnapshotType(descendant.type)) && - descendant.label?.trim() === nodeLabel - ); - }); + return Boolean( + findDescendant(nodes, position, (descendant) => { + const type = normalizeType(descendant.type ?? ''); + return ( + (type === 'link' || type === 'searchfield' || isScrollableSnapshotType(descendant.type)) && + descendant.label?.trim() === nodeLabel + ); + }), + ); } function suppressRepeatedStaticDescendants( - descendants: RawSnapshotNode[], + nodes: RawSnapshotNode[], + position: number, label: string, suppressedIndexes: Set, ): void { - for (const descendant of descendants) { + forEachDescendant(nodes, position, (descendant) => { if (isRepeatedStaticNode(descendant, label)) { suppressedIndexes.add(descendant.index); } - } + }); } function collectIosActionWrapperSuppression( @@ -127,13 +135,14 @@ function collectIosActionWrapperSuppression( suppressedIndexes: Set, ): void { forEachOtherNodeWithLabel(nodes, (node, nodeLabel, position) => { - const semanticDescendant = collectDescendants(nodes, position).find( - (descendant) => + const semanticDescendant = findDescendant(nodes, position, (descendant) => { + return ( isSemanticActionNode(descendant) && descendant.label?.trim() === nodeLabel && (areRectsApproximatelyEqual(descendant.rect, node.rect) || - isIosBackdropDismissWrapper(node, descendant)), - ); + isIosBackdropDismissWrapper(node, descendant)) + ); + }); if (semanticDescendant) { suppressedIndexes.add(node.index); } @@ -193,9 +202,9 @@ function collectIosOffscreenKeyboardSuppression( } suppressedIndexes.add(node.index); suppressOffscreenKeyboardAncestors(node, nodes, suppressedIndexes, screenBottom); - for (const descendant of collectDescendants(nodes, position)) { + forEachDescendant(nodes, position, (descendant) => { suppressedIndexes.add(descendant.index); - } + }); } } @@ -255,8 +264,9 @@ function collectIosSearchToolbarSuppression( if (node.label !== 'Toolbar') { continue; } - const descendants = collectDescendants(nodes, position); - const innerSearch = descendants.find( + const innerSearch = findDescendant( + nodes, + position, (candidate) => normalizeType(candidate.type ?? '') === 'searchfield' && candidate.label === 'Search', ); @@ -276,14 +286,14 @@ function suppressSearchToolbarDescendants( keptSearchIndex: number | null, suppressedIndexes: Set, ): void { - for (const descendant of collectDescendants(nodes, position)) { + forEachDescendant(nodes, position, (descendant) => { if (descendant.index === keptSearchIndex) { - continue; + return; } if (shouldSuppressIosSearchToolbarDescendant(descendant)) { suppressedIndexes.add(descendant.index); } - } + }); } function suppressToolbarAncestors( diff --git a/src/daemon/snapshot-presentation/ios/presentation.test.ts b/src/daemon/snapshot-presentation/ios/presentation.test.ts index 9b280b053..9e8747699 100644 --- a/src/daemon/snapshot-presentation/ios/presentation.test.ts +++ b/src/daemon/snapshot-presentation/ios/presentation.test.ts @@ -423,7 +423,6 @@ test('buildSnapshotState collapses iOS backdrop dismiss wrappers', () => { parentIndex: 0, type: 'Other', label: 'Dismiss', - rect: { x: 0, y: 458, width: 402, height: 416 }, }, { index: 2, diff --git a/src/daemon/snapshot-presentation/ios/rows.ts b/src/daemon/snapshot-presentation/ios/rows.ts index 8b4d47184..554f02127 100644 --- a/src/daemon/snapshot-presentation/ios/rows.ts +++ b/src/daemon/snapshot-presentation/ios/rows.ts @@ -30,15 +30,16 @@ function collectIosRowPresentationForNode( rowLabel: string, context: SnapshotTreeRuleContext, ): void { - const descendants = collectDescendants(nodes, position); const rowType = normalizeType(row.type ?? ''); if (rowType === 'button') { + const descendants = collectDescendants(nodes, position); suppressRepeatedRowDescendants(descendants, rowLabel, context.suppressedIndexes, row); return; } if (rowType !== 'cell') { return; } + const descendants = collectDescendants(nodes, position); if (collectSwitchRowPresentation(descendants, row, rowLabel, context)) { return; } diff --git a/src/daemon/snapshot-presentation/tree.ts b/src/daemon/snapshot-presentation/tree.ts index b9bc073f4..0cbd07860 100644 --- a/src/daemon/snapshot-presentation/tree.ts +++ b/src/daemon/snapshot-presentation/tree.ts @@ -7,24 +7,68 @@ export type SnapshotTreeRuleContext = { }; const RECT_FIELDS = ['x', 'y', 'width', 'height'] as const; +const descendantEndPositionCache = new WeakMap(); export function collectDescendants( nodes: RawSnapshotNode[], startPosition: number, ): RawSnapshotNode[] { - const startDepth = nodes[startPosition]?.depth ?? 0; - const descendants: RawSnapshotNode[] = []; - for (let position = startPosition + 1; position < nodes.length; position += 1) { + const node = nodes[startPosition]; + if (!node) return []; + const endPosition = getDescendantEndPositions(nodes)[startPosition] ?? startPosition + 1; + return nodes.slice(startPosition + 1, endPosition); +} + +export function findDescendant( + nodes: RawSnapshotNode[], + startPosition: number, + predicate: (node: RawSnapshotNode) => boolean, +): RawSnapshotNode | undefined { + const endPosition = getDescendantEndPositions(nodes)[startPosition] ?? startPosition + 1; + for (let position = startPosition + 1; position < endPosition; position += 1) { const node = nodes[position]; - if (!node) { - continue; + if (node && predicate(node)) { + return node; } - if ((node.depth ?? 0) <= startDepth) { - break; + } + return undefined; +} + +export function forEachDescendant( + nodes: RawSnapshotNode[], + startPosition: number, + visitor: (node: RawSnapshotNode) => void, +): void { + const endPosition = getDescendantEndPositions(nodes)[startPosition] ?? startPosition + 1; + for (let position = startPosition + 1; position < endPosition; position += 1) { + const node = nodes[position]; + if (node) { + visitor(node); } - descendants.push(node); } - return descendants; +} + +function getDescendantEndPositions(nodes: RawSnapshotNode[]): number[] { + const cached = descendantEndPositionCache.get(nodes); + if (cached) { + return cached; + } + + const endPositions = new Array(nodes.length); + const stack: Array<{ depth: number; position: number }> = []; + for (const [position, node] of nodes.entries()) { + const depth = node?.depth ?? 0; + while (stack.length > 0 && depth <= stack[stack.length - 1]!.depth) { + const previous = stack.pop()!; + endPositions[previous.position] = position; + } + stack.push({ depth, position }); + } + for (const entry of stack) { + endPositions[entry.position] = nodes.length; + } + descendantEndPositionCache.set(nodes, endPositions); + return endPositions; } function collectAncestors(