diff --git a/packages/command-registry/src/registry.ts b/packages/command-registry/src/registry.ts index ec8d024f6f..359d6c550a 100644 --- a/packages/command-registry/src/registry.ts +++ b/packages/command-registry/src/registry.ts @@ -1234,7 +1234,10 @@ export const RAW_COMMAND_DESCRIPTORS = [ allowSessionlessDefaultDevice: isRecordingStartRequest, }, platformExecution: { kind: 'device-runtime', uses: screenRecordingRuntimePlanUses }, - timeoutPolicy: DEFAULT_TIMEOUT_POLICY, + // A `record stop` export can outlast the request envelope. Resetting the daemon mid-export + // left its recording manifest open with no owner, and every later `record start` on the + // device refused until that exact session ran `record stop`. + timeoutPolicy: PRESERVE_DAEMON_TIMEOUT_POLICY, batchable: true, }, { diff --git a/src/__tests__/command-descriptor-timeout-policy.test.ts b/src/__tests__/command-descriptor-timeout-policy.test.ts index 692d6bc8c4..17fc551931 100644 --- a/src/__tests__/command-descriptor-timeout-policy.test.ts +++ b/src/__tests__/command-descriptor-timeout-policy.test.ts @@ -71,6 +71,9 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { // sessions the daemon owns, so a client-side timeout must not SIGKILL the // daemon mid-create/mid-release and orphan them (and every other provider // session held). + // record joined because a `record stop` export can outlast the envelope: a + // reset mid-export left an ownerless open manifest that refused every later + // `record start` on the device. const preserving = commandDescriptors .filter((descriptor) => descriptor.timeoutPolicy.onTimeout === 'preserve-daemon') .map((descriptor) => descriptor.name); @@ -87,6 +90,7 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { 'lease_release', 'longpress', 'press', + 'record', 'scroll', 'snapshot', 'type', diff --git a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts index f02582c40e..acdd528f7e 100644 --- a/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts +++ b/src/daemon-client/__tests__/daemon-client-timeout-route.test.ts @@ -19,8 +19,9 @@ // (the "unknown-session" case) is the common route the original bug misled. // A design that skips the pkill sweep based on the declared flag alone would // skip real cleanup in the rebound case — the dangerous direction. This test -// proves the sweep always fires for local timeouts, and that the HINT text -// (not the cleanup) is what carries the platform-evidence gating. +// proves the sweep fires for every local timeout except `record` (excluded by +// command, never by platform), and that the HINT text (not the cleanup) is +// what carries the platform-evidence gating. import net from 'node:net'; import http from 'node:http'; @@ -308,3 +309,41 @@ test('a refused timeout fallback preserves the timeout without an unhandled reje server.close(); } }); + +test('a local record stop timeout leaves the exporting daemon and runner alive and names the retry', async () => { + // A match would terminate the runner, which is the recorder on a physical iOS device or macOS. + mockRunCmdSync.mockReturnValue({ exitCode: 0, stdout: '', stderr: '' }); + mockIsDaemon.mockReturnValue(true); + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + const { server, port } = await startHangingSocketServer(); + try { + await assert.rejects( + sendRequest( + { port, pid: 7, token: 'test-token', processStartTime: 'start' }, + { + ...buildRequest('ios'), + session: 'e2e-ios-0', + command: 'record', + positionals: ['stop'], + }, + 'socket', + dummyStatePaths(), + TIMEOUT_MS, + ), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'daemon_transport_timeout'); + assert.match( + error.details?.hint as string, + /^The daemon may still be exporting the recording\. Run agent-device record stop --session e2e-ios-0 again/, + ); + return true; + }, + ); + } finally { + server.close(); + } + assert.equal(mockRunCmdSync.mock.calls.length, 0); + assert.equal(kill.mock.calls.length, 0); + assert.equal(mockStop.mock.calls.length, 0); +}); diff --git a/src/daemon-client/__tests__/daemon-client-timeout.test.ts b/src/daemon-client/__tests__/daemon-client-timeout.test.ts index 1c5658a2d9..46c330c2b6 100644 --- a/src/daemon-client/__tests__/daemon-client-timeout.test.ts +++ b/src/daemon-client/__tests__/daemon-client-timeout.test.ts @@ -97,7 +97,7 @@ test('request timeout hint only names Apple runner cleanup on actual evidence', ); }); -test('a timed-out remote recording names the retry that returns the export', () => { +test('a timed-out record stop on a surviving daemon names the retry that returns the export', () => { assert.equal( resolveRequestTimeoutHint({ remote: true, @@ -107,7 +107,7 @@ test('a timed-out remote recording names the retry that returns the export', () 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.', + 'The remote daemon may still be exporting the recording. Run agent-device record stop --session recording again to wait for that export and receive the completed recording.', ); assert.equal( resolveRequestTimeoutHint({ @@ -117,9 +117,21 @@ test('a timed-out remote recording names the retry that returns the export', () 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.', + 'The remote daemon may still be 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. + // A local daemon preserved across the timeout may still be exporting too. + assert.equal( + resolveRequestTimeoutHint({ + remote: false, + resetDaemon: false, + command: 'record', + appleCleanupEvidence: true, + action: 'stop', + session: 'recording', + }), + 'The daemon may still be exporting the recording. Run agent-device record stop --session recording again to wait for that export and receive the completed recording.', + ); + // A reset daemon is no longer exporting, so no keep-exporting promise is made. assert.equal( resolveRequestTimeoutHint({ remote: false, diff --git a/src/daemon-client/daemon-client-timeout.ts b/src/daemon-client/daemon-client-timeout.ts index d338f74d02..d442b24b3e 100644 --- a/src/daemon-client/daemon-client-timeout.ts +++ b/src/daemon-client/daemon-client-timeout.ts @@ -56,8 +56,8 @@ export function handleRequestTimeout( ): 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 + // Cleanup eligibility never depends on the declared platform, on purpose: + // the request's declared --platform is not // authoritative for session-bound execution. An existing session's real // device platform can silently override a conflicting declared selector // (`applyStripLockPolicy` in request-lock-policy.ts, reached via @@ -67,7 +67,10 @@ export function handleRequestTimeout( // Apple-process-name-specific, so sweeping them on a non-Apple host or // session matches nothing and costs a few no-op subprocess spawns, never // a wrong skip. - const cleanup = remote ? { terminated: 0 } : cleanupTimedOutIosRunnerBuilds(); + // `record` is excluded by command, which is authoritative: on a physical iOS device or macOS the + // runner is the recorder, and the sweep would kill the export the preserved daemon is finishing. + const sweepRunnerBuilds = !remote && command !== PUBLIC_COMMANDS.record; + const cleanup = sweepRunnerBuilds ? cleanupTimedOutIosRunnerBuilds() : { terminated: 0 }; const resetDaemon = !remote && shouldResetDaemonAfterRequestTimeout(command); const daemonReset = resetDaemon ? resetDaemonAfterTimeout(info, statePaths) @@ -136,15 +139,15 @@ export function resolveRequestTimeoutHint(params: { session?: string; }): string { const { remote, resetDaemon, command, appleCleanupEvidence, session, action } = params; + // A daemon that survives this client window may still be exporting a `record stop` that ran out + // of time (a stop still queued for the device lock is dropped before any export starts), and a + // finished file stays retrievable by asking again. A reset daemon makes no such promise. + if (!resetDaemon && command === PUBLIC_COMMANDS.record && action === 'stop') { + return `The ${remote ? 'remote ' : ''}daemon may still be exporting the recording. Run agent-device record stop${ + session ? ` --session ${session}` : '' + } again to wait for that export and receive the completed recording.`; + } 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) { diff --git a/website/docs/docs/commands.md b/website/docs/docs/commands.md index 95e2777289..ecace962c0 100644 --- a/website/docs/docs/commands.md +++ b/website/docs/docs/commands.md @@ -1092,7 +1092,7 @@ agent-device record stop # Stop active recording - Android uses `adb shell screenrecord`, which has a 180s platform limit. `record start` publishes a durable device manifest. Longer recordings are split into MP4 chunks while the daemon stays alive; after daemon restart, `record stop` recovers only manifest-owned chunks and warns when gesture overlay telemetry was lost. - Android `screenrecord` encodes a frame only when the screen changes, so a clip ends at the last frame the recorder encoded instead of at `record stop`: a window that ends on an unchanged screen yields a shorter video, while every on-screen change inside the window stays at its real offset in it. `record stop` reports `durationMs` as host wall clock from `record start` until the export finished, and when the video can be measured it also reports `capturedDurationMs` and warns with how much of the window that video covers. - Limrun iOS and Android direct sessions record the whole simulator or emulator screen through the provider's server-side recorder, so every `--scope` captures the same frame and `--fps` and `--hide-touches` are refused with `INVALID_ARGS` before any device work. `record stop` asks the instance to stop once and then downloads the served MP4 to the output path; that download is bounded to end inside the request window, so a slow or dropped transfer ends typed, leaves no file behind, and is retried by the next `record stop` from the same URL while the instance lives. Nothing survives a daemon restart: the recording is `unreattachable` and the instance disposes the file when the lease is released. -- `record stop` is safe to repeat. When its request window ends while the daemon is still exporting — typical for a long touch-overlay burn-in on a remote daemon — the export keeps running there, and a second `record stop` in the same session returns that completed recording, including the caller-side output path, without starting another recording. A finished recording whose video file is already gone reports `no active recording`. +- `record stop` is safe to repeat. When its request window ends while the daemon is still exporting — typical for a long touch-overlay burn-in, on a local or remote daemon — the export keeps running there, and a second `record stop` in the same session returns that completed recording, including the caller-side output path, without starting another recording. A finished recording whose video file is already gone reports `no active recording`. - A recording answers two independent questions, and `record stop` reports both: whether a playable export exists, and whether the recorder stopped. `recorder` is `confirmed` when the recorder exited or acknowledged a stop meant for this recording, and `lost` when the session holding it died — an Apple recording invalidated by a runner restart. `nativePathDisposition` says what became of the artifact path the recorder itself writes to: `retirable` while that file still sits there owed a removal, and `retired` once its removal was verified. Both are optional disclosures — the export is served either way, a replay of a recording stopped before they existed omits them, and so does a backend whose recorder writes the served file itself. The vocabulary is deliberately wider than today's answers: `recorder: unconfirmed` (a probe that could not be read, or no exit inside the stop budget), the identity-mismatch reasons under `lost`, and `nativePathDisposition: pending` are declared by [ADR 0024](https://github.com/callstack/agent-device/blob/main/docs/adr/0024-screen-recording-provable-signal.md) for the steps that gain those probes, and no stop reports them yet. - `record contact-sheet [--out ]` turns a recording you already exported into one PNG: the frames where the screen visibly changed, laid out in a grid and each labeled with its elapsed time (`HH:MM:SS.mmm`) on the clip timeline. It is how an agent reads a recording it cannot play. It reads the file you pass — no session, no device, no daemon — so it can rebuild an old take and the sheet can only describe screens that export contains. Any backend that exports MP4 works (Apple, Android, HarmonyOS, Limrun); a WebM recording from the web backend is refused with `details.reason: contact_sheet_container_unsupported`. The default output is `.contact-sheet.png`, and `--out` is refused when it points back at the recording itself, because a sheet is derived from a take and never replaces it. `--json` reports each cell's time and changed-pixel share alongside `sampledFrameCount`, `decodedFrameCount`, and `skippedSampleCount`.