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
6 changes: 5 additions & 1 deletion packages/contracts/src/facades/recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ export {
recordingQualityInputToExportQuality,
} from '../recording-export-quality.ts';
export type { RecordingExportQuality } from '../recording-export-quality.ts';
export { RECORDING_SCOPE_VALUES, isWholeScreenRecordingScope } from '../recording-scope.ts';
export {
RECORDING_SCOPE_VALUES,
isRecordingScope,
isWholeScreenRecordingScope,
} from '../recording-scope.ts';
export type { RecordingScope } from '../recording-scope.ts';
export type {
RecordingAppIdentity,
Expand Down
4 changes: 4 additions & 0 deletions packages/contracts/src/recording-scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ export const RECORDING_SCOPE_VALUES = ['app', 'device', 'system'] as const;

export type RecordingScope = (typeof RECORDING_SCOPE_VALUES)[number];

export function isRecordingScope(value: unknown): value is RecordingScope {
return RECORDING_SCOPE_VALUES.some((scope) => scope === value);
}

export function isWholeScreenRecordingScope(scope: RecordingScope): boolean {
return scope === 'device' || scope === 'system';
}
2 changes: 1 addition & 1 deletion src/cli-schema/cli-help.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ Batch:
agent-device test ./e2e/maestro --maestro --device udid1,emulator-5554 --shard-all 2

Recording:
record start/stop. Default scope is app (needs an active open session); use --scope device/system for whole-screen capture spanning multiple apps/home/settings. --quality medium|high on Android and Apple targets. stop burns touch overlays into the video by default; --hide-touches skips that for the fastest raw recording, and is recommended for gesture-heavy iOS simulator proof videos since overlay timing depends on a stable runner session. Android adb screenrecord has a 180s limit, so long Android recordings return as multiple MP4 chunks while the daemon stays alive; after a daemon restart, record stop recovers only manifest-owned chunks.
record start/stop. Default scope is app (needs an active open session); use --scope device/system for whole-screen capture spanning multiple apps/home/settings. --quality medium|high on Android and Apple targets. stop burns touch overlays into the video by default; --hide-touches skips that for the fastest raw recording, and is recommended for gesture-heavy iOS simulator proof videos since overlay timing depends on a stable runner session. Android adb screenrecord has a 180s limit, so long Android recordings return as multiple MP4 chunks while the daemon stays alive; after a daemon restart, record stop recovers only manifest-owned chunks. record stop is safe to repeat: if its request window ended while the daemon was still exporting, running it again in that session returns the completed recording instead of starting a second one.
Tracing: trace start ./trace.log, trace stop ./trace.log (path is positional, not --path).`,
},
gestures: {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Production-seam coverage for the real request-timeout route.
//
// src/daemon-client/__tests__/daemon-client.test.ts covers `resolveRequestTimeoutHint`
// as a pure formatter, but a pure-formatter test cannot catch a bug in
// src/daemon-client/__tests__/daemon-client-timeout.test.ts covers
// `resolveRequestTimeoutHint` as a pure formatter, but a pure-formatter test cannot catch a bug in
// CLEANUP ELIGIBILITY: whether `cleanupTimedOutIosRunnerBuilds` (the Apple
// xcodebuild pkill sweep) actually runs. This file spies on the real
// process-execution seam (`runCmdSync`, @agent-device/host-kit/command) and drives an
Expand Down
146 changes: 146 additions & 0 deletions src/daemon-client/__tests__/daemon-client-timeout.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// The pure hint formatter in src/daemon-client/daemon-client-timeout.ts: what a timed-out request
// tells the caller to do next. daemon-client-timeout-route.test.ts covers the same route at its
// production seam, where cleanup eligibility is decided; these assertions only fix the wording.

import assert from 'node:assert/strict';
import { test } from 'vitest';
import { resolveRequestTimeoutHint } from '../daemon-client-timeout.ts';

test('request timeout hint only names Apple runner cleanup on actual evidence', () => {
// Before this change, handleRequestTimeout emitted Apple-specific hint
// wording for EVERY local timeout, regardless of the request's
// --platform. That was misleading for Android/web/Harmony sessions, which
// never had any Apple runner work to abort.
//
// The fix is evidence-based, not platform-guess-based:
// `appleCleanupEvidence` is true only when the request declared an
// AFFIRMATIVELY Apple platform (apple/ios/macos) or the pkill cleanup
// itself terminated a matching process — never from an undeclared or
// declared-non-Apple platform alone. (Why not trust the declared platform
// directly: it is not authoritative for session-bound execution — see
// `handleRequestTimeout`'s comment and the production-seam coverage in
// daemon-client-timeout-route.test.ts for the cleanup-eligibility half of
// this contract that this pure formatter test cannot prove.)

// appleCleanupEvidence: true keeps the exact historical wording — nothing
// regresses for the true-Apple case.
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out press request was canceled and Apple runner work was aborted when detected; the daemon was kept alive so the session can still be closed or inspected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: true,
command: 'open',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. Timed-out Apple runner xcodebuild processes were terminated when detected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'snapshot',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out snapshot request was canceled and Apple runner work was aborted when detected; the daemon was kept alive so the session can still be closed or inspected. If this was the first Apple-platform snapshot on the device, run agent-device prepare ios-runner with the same --platform before snapshot/test so runner startup is handled explicitly.',
);

// appleCleanupEvidence: false — no Apple-runner claim in any branch, and
// the Apple-only iOS-prepare follow-up drops entirely. This is the
// motivating fix: it fires equally whether the platform was declared
// non-Apple OR left undeclared (the common session-bound case), because
// neither is Apple evidence on its own.
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out press request was canceled; the daemon was kept alive so the session can still be closed or inspected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: true,
command: 'open',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The daemon was reset after the timeout.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'snapshot',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out snapshot request was canceled; the daemon was kept alive so the session can still be closed or inspected.',
);

// Remote requests were never Apple-specific and stay evidence-independent.
assert.equal(
resolveRequestTimeoutHint({
remote: true,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: false,
}),
'Retry with --debug and verify the remote daemon URL, auth token, and remote host logs.',
);
});

test('a timed-out remote recording names the retry that returns the export', () => {
assert.equal(
resolveRequestTimeoutHint({
remote: true,
resetDaemon: false,
command: 'record',
appleCleanupEvidence: false,
action: 'stop',
session: 'recording',
}),
'The remote daemon is still exporting the recording. Run agent-device record stop --session recording again to wait for that export and receive the completed recording.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: true,
resetDaemon: false,
command: 'record',
appleCleanupEvidence: false,
action: 'stop',
}),
'The remote daemon is still exporting the recording. Run agent-device record stop again to wait for that export and receive the completed recording.',
);
// A local timeout resets the daemon mid-export, so no keep-exporting promise is made.
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: true,
command: 'record',
appleCleanupEvidence: false,
action: 'stop',
session: 'recording',
}),
'Retry with --debug and check daemon diagnostics logs. The daemon was reset after the timeout.',
);
// `record start` runs no export, so it keeps the generic remote wording.
assert.equal(
resolveRequestTimeoutHint({
remote: true,
resetDaemon: false,
command: 'record',
appleCleanupEvidence: false,
action: 'start',
session: 'recording',
}),
'Retry with --debug and verify the remote daemon URL, auth token, and remote host logs.',
);
});
96 changes: 1 addition & 95 deletions src/daemon-client/__tests__/daemon-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ import {
} from '../daemon-client-metadata.ts';
import { canConnectSocket } from '../daemon-client-transport.ts';
import { DAEMON_RPC_PROTOCOL_VERSION } from '@agent-device/contracts/daemon-http';
import {
resolveRequestTimeoutHint,
shouldResetDaemonAfterRequestTimeout,
} from '../daemon-client-timeout.ts';
import { shouldResetDaemonAfterRequestTimeout } from '../daemon-client-timeout.ts';
import { resolveDaemonPaths } from '../../daemon/config.ts';
import { stopProcessForTakeover } from '../../daemon/daemon-process.ts';
import { findProjectRoot, readVersion } from '@agent-device/host-kit/version';
Expand Down Expand Up @@ -251,97 +248,6 @@ test('read-only polling command timeouts preserve the daemon like snapshot', ()
assert.equal(shouldResetDaemonAfterRequestTimeout('open'), true);
});

test('request timeout hint only names Apple runner cleanup on actual evidence', () => {
// Before this change, handleRequestTimeout emitted Apple-specific hint
// wording for EVERY local timeout, regardless of the request's
// --platform. That was misleading for Android/web/Harmony sessions, which
// never had any Apple runner work to abort.
//
// The fix is evidence-based, not platform-guess-based:
// `appleCleanupEvidence` is true only when the request declared an
// AFFIRMATIVELY Apple platform (apple/ios/macos) or the pkill cleanup
// itself terminated a matching process — never from an undeclared or
// declared-non-Apple platform alone. (Why not trust the declared platform
// directly: it is not authoritative for session-bound execution — see
// `handleRequestTimeout`'s comment and the production-seam coverage in
// daemon-client-timeout-route.test.ts for the cleanup-eligibility half of
// this contract that this pure formatter test cannot prove.)

// appleCleanupEvidence: true keeps the exact historical wording — nothing
// regresses for the true-Apple case.
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out press request was canceled and Apple runner work was aborted when detected; the daemon was kept alive so the session can still be closed or inspected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: true,
command: 'open',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. Timed-out Apple runner xcodebuild processes were terminated when detected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'snapshot',
appleCleanupEvidence: true,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out snapshot request was canceled and Apple runner work was aborted when detected; the daemon was kept alive so the session can still be closed or inspected. If this was the first Apple-platform snapshot on the device, run agent-device prepare ios-runner with the same --platform before snapshot/test so runner startup is handled explicitly.',
);

// appleCleanupEvidence: false — no Apple-runner claim in any branch, and
// the Apple-only iOS-prepare follow-up drops entirely. This is the
// motivating fix: it fires equally whether the platform was declared
// non-Apple OR left undeclared (the common session-bound case), because
// neither is Apple evidence on its own.
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out press request was canceled; the daemon was kept alive so the session can still be closed or inspected.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: true,
command: 'open',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The daemon was reset after the timeout.',
);
assert.equal(
resolveRequestTimeoutHint({
remote: false,
resetDaemon: false,
command: 'snapshot',
appleCleanupEvidence: false,
}),
'Retry with --debug and check daemon diagnostics logs. The timed-out snapshot request was canceled; the daemon was kept alive so the session can still be closed or inspected.',
);

// Remote requests were never Apple-specific and stay evidence-independent.
assert.equal(
resolveRequestTimeoutHint({
remote: true,
resetDaemon: false,
command: 'press',
appleCleanupEvidence: false,
}),
'Retry with --debug and verify the remote daemon URL, auth token, and remote host logs.',
);
});

test('cleanupFailedDaemonStartupMetadata removes partial startup metadata', async () => {
const stateDir = mkdtempForTestSync('agent-device-daemon-cleanup-');
const paths = resolveDaemonPaths(stateDir);
Expand Down
43 changes: 34 additions & 9 deletions src/daemon-client/daemon-client-timeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,21 @@ function isAffirmativelyApplePlatform(platform: PlatformSelector | undefined): b
}

export function handleRequestTimeout(
info: DaemonInfo,
statePaths: DaemonPaths,
requestId: string | undefined,
command: string | undefined,
remote: boolean,
timeoutMs: number,
platform: PlatformSelector | undefined,
params: Readonly<{
info: DaemonInfo;
statePaths: DaemonPaths;
remote: boolean;
timeoutMs: number;
requestId: string | undefined;
command: string | undefined;
platform: PlatformSelector | undefined;
/** Named together so the recovery hint cannot be assembled from a swapped session and action. */
session?: string;
action?: string;
}>,
): AppError {
const { info, statePaths, remote, timeoutMs, requestId, command, platform, session, action } =
params;
// Cleanup eligibility stays UNCONDITIONAL for every local (non-remote)
// timeout, on purpose: the request's declared --platform is not
// authoritative for session-bound execution. An existing session's real
Expand Down Expand Up @@ -87,7 +94,14 @@ export function handleRequestTimeout(
return new AppError('COMMAND_FAILED', 'Daemon request timed out', {
timeoutMs,
requestId,
hint: resolveRequestTimeoutHint({ remote, resetDaemon, command, appleCleanupEvidence }),
hint: resolveRequestTimeoutHint({
remote,
resetDaemon,
command,
appleCleanupEvidence,
session,
action,
}),
});
}

Expand All @@ -112,9 +126,20 @@ export function resolveRequestTimeoutHint(params: {
resetDaemon: boolean;
command: string | undefined;
appleCleanupEvidence: boolean;
/** The request's first positional, for commands whose recovery depends on which action ran. */
action?: string;
session?: string;
}): string {
const { remote, resetDaemon, command, appleCleanupEvidence } = params;
const { remote, resetDaemon, command, appleCleanupEvidence, session, action } = params;
if (remote) {
// A remote daemon survives this client window, so a `record stop` that ran out of time is still
// exporting there and its finished file stays retrievable by asking again. A local timeout
// resets the daemon mid-export, where that promise would be false.
if (command === PUBLIC_COMMANDS.record && action === 'stop') {
return `The remote daemon is still exporting the recording. Run agent-device record stop${
session ? ` --session ${session}` : ''
} again to wait for that export and receive the completed recording.`;
}
return 'Retry with --debug and verify the remote daemon URL, auth token, and remote host logs.';
}
if (!resetDaemon) {
Expand Down
Loading
Loading