fix: clean up recording stop and snapshot traversal (#640)

Author: thymikee

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

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 {
@@ -413,8 +413,9 @@ export async function stopAndroidRecording(params: {
   deps: RecordTraceDeps;
   device: AndroidDevice;
   recording: AndroidRecording;
+  stopRequestedAt: number;
 }): Promise<DaemonResponse | null> {
-  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;
+}

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<NonNullable<SessionState['recording']>, { platform: 'ios' | 'android' }>;
+  stopRequestedAt: number;
 }): Promise<DaemonResponse | null> {
-  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<NonNullable<SessionState['recording']>, { 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;
   }

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