Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 137 additions & 0 deletions src/daemon/handlers/__tests__/record-trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();

Expand Down Expand Up @@ -183,6 +186,8 @@ beforeEach(() => {
mockResizeRecording.mockImplementation(async () => {});
mockTrimRecordingStart.mockImplementation(async () => {});
mockOverlayRecordingTouches.mockImplementation(async () => {});
mockWaitForStableFile.mockImplementation(async () => {});
mockIsPlayableVideo.mockImplementation(async () => true);
});

afterEach(() => {
Expand Down Expand Up @@ -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';
Expand Down
15 changes: 10 additions & 5 deletions src/daemon/handlers/record-trace-android-copy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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';
}

Expand All @@ -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.
}
}
23 changes: 19 additions & 4 deletions src/daemon/handlers/record-trace-android.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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 });
Expand All @@ -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) {
Expand Down Expand Up @@ -488,3 +495,11 @@ export async function stopAndroidRecording(params: {
}
}
}

function formatAndroidStopFailure(
error: string,
recording: AndroidRecording,
stopRequestedAt: number,
): string {
return buildRecordStopFailure(error, recording, stopRequestedAt).message;
}
47 changes: 37 additions & 10 deletions src/daemon/handlers/record-trace-recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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), {
Expand All @@ -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,
);
}

Expand All @@ -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,
);
}

Expand Down Expand Up @@ -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;
Expand All @@ -412,6 +438,7 @@ async function stopRecording(params: {
}

const recording = activeSession.recording;
const stopRequestedAt = Date.now();
const invalidatedReason = recording.invalidatedReason;
activeSession.recording = undefined;

Expand All @@ -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;
}
Expand Down
26 changes: 26 additions & 0 deletions src/daemon/record-trace-errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Measure short recordings at stop request time

Because this defaults now to the time the error is formatted, the short-recording check can miss the exact case it is meant to explain. For example, a user can run record stop immediately, but the iOS simulator path reaches this helper only after waitForStableFile/isPlayableVideo, and the Android path waits for remote stability plus pull validation before formatAndroidStopFailure; those waits can push elapsed time past 1000ms even though the requested recording duration was under the limit, so the actionable “wait at least 1000ms” hint and too-short cleanup are skipped. Capture the stop-request timestamp before finalization/validation waits and pass it into this helper.

Useful? React with 👍 / 👎.

): 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,
};
}
Loading
Loading