fix: clean up recording stop and snapshot traversal

Author: thymikee

src/daemon/handlers/__tests__/record-trace.test.ts

@@ -596,6 +596,41 @@ 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 start stores iOS simulator recorder pid for scoped cleanup', async () => {
   const sessionStore = makeSessionStore();
   const sessionName = 'ios-sim-recorder-pid';

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.
+  }
+}

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 {
@@ -444,7 +444,7 @@ export async function stopAndroidRecording(params: {
     });
     if (copyError) {
       await cleanupRemoteRecording();
-      return errorResponse('COMMAND_FAILED', copyError);
+      return errorResponse('COMMAND_FAILED', formatAndroidStopFailure(copyError, recording));
     }
 
     await finalizeAndroidRecordingOutput({ recording, deps });
@@ -453,7 +453,7 @@ export async function stopAndroidRecording(params: {
   await cleanupRemoteRecording();
 
   if (stopError) {
-    return errorResponse('COMMAND_FAILED', stopError);
+    return errorResponse('COMMAND_FAILED', formatAndroidStopFailure(stopError, recording));
   }
 
   if (cleanupError) {
@@ -488,3 +488,7 @@ export async function stopAndroidRecording(params: {
     }
   }
 }
+
+function formatAndroidStopFailure(error: string, recording: AndroidRecording): string {
+  return buildRecordStopFailure(error, recording).message;
+}

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';
@@ -328,9 +332,9 @@ async function stopNonRunnerRecording(params: {
     );
   }
   if (stopResult.exitCode !== 0) {
-    return errorResponse(
-      'COMMAND_FAILED',
+    return buildLocalRecordingStopFailure(
       `failed to stop recording: ${formatRecordTraceExecFailure(stopResult, 'simctl recordVideo')}`,
+      recording,
     );
   }
 
@@ -353,9 +357,9 @@ async function stopNonRunnerRecording(params: {
     },
   );
   if (!playable) {
-    return errorResponse(
-      'COMMAND_FAILED',
+    return buildLocalRecordingStopFailure(
       `failed to stop recording: ${recording.outPath} was not finalized into a playable video`,
+      recording,
     );
   }
 
@@ -398,6 +402,25 @@ async function stopNonRunnerRecording(params: {
   return null;
 }
 
+function buildLocalRecordingStopFailure(
+  message: string,
+  recording: Extract<NonNullable<SessionState['recording']>, { platform: 'ios' | 'android' }>,
+): DaemonResponse {
+  const failure = buildRecordStopFailure(message, recording);
+  if (failure.tooShort) {
+    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;

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,
+  };
+}

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,59 +86,66 @@ function collectRepeatedStaticSuppressionForNode(
   nodeLabel: string,
   suppressedIndexes: Set<number>,
 ): 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<number>,
 ): void {
-  for (const descendant of descendants) {
+  forEachDescendant(nodes, position, (descendant) => {
     if (isRepeatedStaticNode(descendant, label)) {
       suppressedIndexes.add(descendant.index);
     }
-  }
+  });
 }
 
 function collectIosActionWrapperSuppression(
   nodes: RawSnapshotNode[],
   suppressedIndexes: Set<number>,
 ): void {
   forEachOtherNodeWithLabel(nodes, (node, nodeLabel, position) => {
-    const semanticDescendant = collectDescendants(nodes, position).find(
-      (descendant) =>
+    if (!node.rect) {
+      return;
+    }
+    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 +205,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 +267,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 +289,14 @@ function suppressSearchToolbarDescendants(
   keptSearchIndex: number | null,
   suppressedIndexes: Set<number>,
 ): 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(

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;
   }