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
159 changes: 134 additions & 25 deletions packages/capture-kit/src/recording/video.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,18 @@ import { createTestHost } from '../host-environment.fixtures.ts';
import path from 'node:path';
import { beforeEach, expect, test, vi } from 'vitest';
import { runCmd } from '@agent-device/host-kit/command';
import { AppError } from '@agent-device/kernel/errors';
import { AppError, normalizeError } from '@agent-device/kernel/errors';
import { likelyPlayableWebmContainer } from '../__tests__/test-utils/video-fixtures.ts';
import { mkdtempForTestSync } from '../tmp-dir.fixtures.ts';
import { mp4Atom, mp4MovieHeader } from './mp4.fixtures.ts';
import { Deadline } from '@agent-device/host-kit/retry';
import { compileSwiftSourceText } from './swift-cache.ts';
import { hasVideoContainer, isPlayableVideo, waitForPlayableVideo } from './video.ts';
import {
hasVideoContainer,
isPlayableVideo,
prepareVideoValidator,
waitForPlayableVideo,
} from './video.ts';

vi.mock(import('@agent-device/host-kit/command'), async (importOriginal) => ({
...(await importOriginal()),
Expand Down Expand Up @@ -104,54 +109,158 @@ test('callers share one validator compile, and each waits for it within its own
expect(compileSwiftSourceText).toHaveBeenCalledOnce();
const [compile] = vi.mocked(compileSwiftSourceText).mock.calls[0]!;
expect(compile.timeoutMs).toBeUndefined();
expect(compile.signal?.aborted).toBe(false);
expect(compile.signal).toBeUndefined();
} finally {
vi.useRealTimers();
}
});

test('the last caller to stop waiting cancels an unfinished compile, and the next caller compiles again', async () => {
// Nothing owns swiftc once its requests end: a daemon that exits while it runs leaves it
// reparented to launchd, and the compile's own temp output behind.
const mp4 = writeFixture('cancel-compile.mp4', Buffer.concat([ftyp, moov]));
const host = { env: { AGENT_DEVICE_SWIFT_CACHE_DIR: '/cache/cancel' }, cwd: directory };
test('an expired waiter leaves the compile running for the next stop', async () => {
const mp4 = writeFixture('pending-compile.mp4', Buffer.concat([ftyp, moov]));
const host = { env: { AGENT_DEVICE_SWIFT_CACHE_DIR: '/cache/pending' }, cwd: directory };
let resolveCompile!: (value: string) => void;
const pending = new Promise<string>((resolve) => {
resolveCompile = resolve;
});
vi.mocked(compileSwiftSourceText).mockClear().mockReturnValueOnce(pending);
void prepareVideoValidator(host);
await hasVideoContainer(mp4);
let compileSignal: AbortSignal | undefined;
vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout', 'Date'] });
try {
vi.mocked(compileSwiftSourceText)
.mockClear()
.mockImplementationOnce(async ({ signal }) => {
compileSignal = signal;
return await new Promise<string>((_, reject) => {
signal?.addEventListener('abort', () => reject(signal.reason));
});
});

const timedOut = expect(
isPlayableVideo(mp4, host, Deadline.fromTimeoutMs(5_000)),
).rejects.toMatchObject({ details: { reason: 'video_validator_compile_timeout' } });
).rejects.toMatchObject({
details: { reason: 'video_validator_compile_timeout', retriable: true },
});
await vi.advanceTimersByTimeAsync(5_000);
await timedOut;
expect(compileSignal?.aborted).toBe(true);
expect(runCmd).not.toHaveBeenCalled();
resolveCompile('/bin/true');
await expect(isPlayableVideo(mp4, host)).resolves.toBe(true);
expect(compileSwiftSourceText).toHaveBeenCalledOnce();
} finally {
resolveCompile('/bin/true');
vi.useRealTimers();
}

await expect(isPlayableVideo(mp4, host)).resolves.toBe(true);
expect(compileSwiftSourceText).toHaveBeenCalledTimes(2);
});

test('a failed compile is dropped, and the next caller compiles again', async () => {
test('a rejected compile is evicted so the next attempt compiles and verifies', async () => {
const mp4 = writeFixture('failed-compile.mp4', Buffer.concat([ftyp, moov]));
const host = { env: { AGENT_DEVICE_SWIFT_CACHE_DIR: '/cache/failed' }, cwd: directory };
vi.mocked(compileSwiftSourceText)
.mockClear()
.mockRejectedValueOnce(new AppError('COMMAND_FAILED', 'xcrun timed out after 120000ms'));

await expect(isPlayableVideo(mp4, host)).rejects.toMatchObject({ code: 'COMMAND_FAILED' });
await expect(prepareVideoValidator(host)).rejects.toMatchObject({
details: { reason: 'video_validator_unavailable' },
});
await expect(isPlayableVideo(mp4, host)).resolves.toBe(true);
expect(compileSwiftSourceText).toHaveBeenCalledTimes(2);
expect(runCmd).toHaveBeenCalledOnce();
});

test('preserves compiler stderr and recovery hint without replacing the typed validator reason', async () => {
const mp4 = writeFixture('compiler-diagnostics.mp4', Buffer.concat([ftyp, moov]));
const host = {
env: { AGENT_DEVICE_SWIFT_CACHE_DIR: '/cache/compiler-diagnostics' },
cwd: directory,
};
const stderr = 'xcrun: error: unable to find utility "swiftc", not a developer tool or in PATH';
const hint =
'Export DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer before running agent-device.';
const failure = new AppError('COMMAND_FAILED', 'xcrun exited with code 72', {
stderr,
stdout: '',
exitCode: 72,
reason: 'xcrun-tool-not-found',
hint,
});
vi.mocked(compileSwiftSourceText).mockRejectedValueOnce(failure);
await expect(isPlayableVideo(mp4, host)).rejects.toMatchObject({
code: 'COMMAND_FAILED',
details: {
reason: 'video_validator_unavailable',
retriable: false,
stderr,
stdout: '',
exitCode: 72,
hint,
},
cause: failure,
});
});

test('retains main container-only completion when the Swift compiler is missing', async () => {
const mp4 = writeFixture('missing-compiler.mp4', Buffer.concat([ftyp, moov]));
const host = { env: { AGENT_DEVICE_SWIFT_CACHE_DIR: '/cache/missing-compiler' }, cwd: directory };
vi.mocked(compileSwiftSourceText).mockRejectedValueOnce(
new AppError('TOOL_MISSING', 'xcrun not found in PATH'),
);
await expect(isPlayableVideo(mp4, host)).resolves.toBe(true);
expect(runCmd).not.toHaveBeenCalled();
});

test('a missing validator executable after compilation returns the typed unavailable error', async () => {
const mp4 = writeFixture('missing-validator.mp4', Buffer.concat([ftyp, moov]));
vi.mocked(runCmd).mockRejectedValueOnce(
new AppError('TOOL_MISSING', 'validator not found in PATH'),
);
await expect(isPlayableVideo(mp4, createTestHost())).rejects.toMatchObject({
code: 'COMMAND_FAILED',
details: { reason: 'video_validator_unavailable', retriable: false },
});
});

test.each([
{
stderr: `xcrun: error: unable to find utility "swiftc", not a developer tool or in PATH`,
stdout: '',
recoveryHint: 'DEVELOPER_DIR',
messageExcerpt: 'unable to find utility "swiftc"',
},
{
stderr: `validator.swift:2:8: error: no such module 'AVFoundation'`,
stdout: '',
recoveryHint: 'AVFoundation',
messageExcerpt: "no such module 'AVFoundation'",
},
{
stderr: '',
stdout: `validator.swift:2:8: error: no such module 'AVFoundation'`,
recoveryHint: 'AVFoundation',
messageExcerpt: 'MP4 playability could not be verified.',
},
])(
'preserves unavailable validator output and selects its recovery hint: %j',
async ({ stderr, stdout, recoveryHint, messageExcerpt }) => {
const mp4 = writeFixture('unavailable.mp4', Buffer.concat([ftyp, moov]));
vi.mocked(runCmd).mockResolvedValueOnce({ exitCode: 1, stderr, stdout });
const playable = isPlayableVideo(mp4, createTestHost());
await expect(playable).rejects.toMatchObject({
code: 'COMMAND_FAILED',
details: {
reason: 'video_validator_unavailable',
retriable: false,
stderr,
stdout,
exitCode: 1,
hint: expect.stringContaining(recoveryHint),
},
});
await expect(playable.catch(normalizeError)).resolves.toMatchObject({
message: expect.stringContaining(messageExcerpt),
});
},
);

test('an ordinary validator refusal is unplayable, not unavailable', async () => {
const mp4 = writeFixture('refused.mp4', Buffer.concat([ftyp, moov]));
vi.mocked(runCmd).mockResolvedValueOnce({
exitCode: 1,
stderr: "no such module 'OtherModule'",
stdout: '',
});
await expect(isPlayableVideo(mp4, createTestHost())).resolves.toBe(false);
});

test('a budget spent before the validator runs reads as not playable instead of a 1 ms run', async () => {
Expand Down
123 changes: 64 additions & 59 deletions packages/capture-kit/src/recording/video.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fs from 'node:fs';
import { AppError } from '@agent-device/kernel/errors';
import { isCommandTimeoutError, runCmd } from '@agent-device/host-kit/command';
import { execFailureDetails, isCommandTimeoutError, runCmd } from '@agent-device/host-kit/command';
import { type Deadline, sleep } from '@agent-device/host-kit/retry';
import { buildSwiftToolEnv, compileSwiftSourceText, getSwiftCacheRoot } from './swift-cache.ts';
import type { HostEnvironment } from '@agent-device/kernel/source-value';
Expand Down Expand Up @@ -40,19 +40,26 @@
const VALIDATOR_COMPILE_WAIT_MS = 30_000;
const VALIDATOR_RUN_TIMEOUT_MS = 10_000;

type SharedValidatorCompile = {
readonly compiled: Promise<string>;
readonly cancel: AbortController;
waiters: number;
settled: boolean;
};
const videoValidatorCompiles = new Map<string, Promise<string>>();

// A compiled helper is valid for the Swift cache it was built into, so each cache root keeps its own.
// Callers share one compile, and each waits for it only within its own budget. The last caller to
// stop waiting cancels an unfinished compile: that kills this process's `swiftc`, or stops waiting
// for another process's compile. Nothing owns `swiftc` once its requests end, so a daemon that
// exits would leave it running.
const videoValidatorCompiles = new Map<string, SharedValidatorCompile>();
/** Starts one cached compile per Swift cache root. Callers may warm it without awaiting it. */
export function prepareVideoValidator(host: HostEnvironment): Promise<string> {
const cacheRoot = getSwiftCacheRoot(host.env);
let compiled = videoValidatorCompiles.get(cacheRoot);
if (!compiled) {
compiled = compileSwiftSourceText({
host,
source: VIDEO_VALIDATION_SCRIPT,
cacheName: 'video-validator',
}).catch((error: unknown) => {
if (videoValidatorCompiles.get(cacheRoot) === compiled)
videoValidatorCompiles.delete(cacheRoot);
throw unavailableVideoValidator(error);
});
videoValidatorCompiles.set(cacheRoot, compiled);
Comment thread
thymikee marked this conversation as resolved.
Resolved
}
return compiled;
}
Comment on lines +43 to +62

@apex-sentinel-staging apex-sentinel-staging Bot Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No longer raised on 823b8d1: the code it pointed at changed.

🟠 Major · One transient compile failure disables MP4 verification for the daemon's lifetime

A transient failure in ensureSwiftExecutable (Swift cache lock held by another process, or xcrun swiftc hitting its 120 s timeout) permanently disables verification: the rejected promise is cached in videoValidatorCompiles under that cache root and never removed, so every subsequent call rejects with video_validator_unavailable and retriable: false for the daemon's lifetime. The predecessor deleted the failed entry so the next caller retried; the new test locks in the no-retry behavior.

Suggested change
const videoValidatorCompiles = new Map<string, Promise<string>>();
// A compiled helper is valid for the Swift cache it was built into, so each cache root keeps its own.
// Callers share one compile, and each waits for it only within its own budget. The last caller to
// stop waiting cancels an unfinished compile: that kills this process's `swiftc`, or stops waiting
// for another process's compile. Nothing owns `swiftc` once its requests end, so a daemon that
// exits would leave it running.
const videoValidatorCompiles = new Map<string, SharedValidatorCompile>();
/** Starts one cached compile per Swift cache root. Callers may warm it without awaiting it. */
export function prepareVideoValidator(host: HostEnvironment): Promise<string> {
const cacheRoot = getSwiftCacheRoot(host.env);
let compiled = videoValidatorCompiles.get(cacheRoot);
if (!compiled) {
compiled = compileSwiftSourceText({
host,
source: VIDEO_VALIDATION_SCRIPT,
cacheName: 'video-validator',
}).catch((error: unknown) => {
throw unavailableVideoValidator(error);
});
videoValidatorCompiles.set(cacheRoot, compiled);
}
return compiled;
}
const videoValidatorCompiles = new Map<string, Promise<string>>();
/** Starts one cached compile per Swift cache root. Callers may warm it without awaiting it. */
export function prepareVideoValidator(host: HostEnvironment): Promise<string> {
const cacheRoot = getSwiftCacheRoot(host.env);
let compiled = videoValidatorCompiles.get(cacheRoot);
if (!compiled) {
compiled = compileSwiftSourceText({
host,
source: VIDEO_VALIDATION_SCRIPT,
cacheName: 'video-validator',
}).catch((error: unknown) => {
// Only a toolchain that cannot provide AVFoundation is permanently unavailable; a lock or
// compile timeout must let the next caller try again.
if (videoValidatorCompiles.get(cacheRoot) === compiled) videoValidatorCompiles.delete(cacheRoot);
throw unavailableVideoValidator(error);
});
videoValidatorCompiles.set(cacheRoot, compiled);
}
return compiled;
}

Suggested change

Drop the map entry when the failure is transient (anything not isSwiftVideoValidatorError), re-check isExecutableFile/the cache before refusing so an executable another process published later is usable, keep unconditional refusal only for a confirmed missing toolchain, and consider retriable: true for the transient class.

Details

Also raised here

Also raised at this location by persona maintainability: A transient validator compile failure poisons the cache for the whole daemon lifetime — If the one compile started at record start fails for a transient reason — the daemon under load hits the xcrun timeout, the Swift cache lock is held by a sibling process, an I/O hiccup — every later record stop in that daemon rejects with video_validator_unavailable, retriable: false, and the hint "Install the Swift toolchain with AVFoundation", which is wrong advice for a timeout and never clears short of a daemon restart. The rejected promise is what gets stored in videoValidatorCompiles, so the map caches the failure permanently; the predecessor deleted the entry on rejection (if (videoValidatorCompiles.get(cacheRoot) === shared) videoValidatorCompiles.delete(cacheRoot)) so the next caller recompiled. Keep the entry only on success, or store a wrapper that clears the key on rejection, so a retriable cause is retried at the next stop.

persona: senior-developer, javascript, maintainability

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

Comment on lines +46 to +62

@apex-sentinel-staging apex-sentinel-staging Bot Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No longer raised on 666fbf1, with the code it pointed at unchanged: withdrawn, not fixed.

🆕 🟡 Minor · Background validator compile is no longer cancellable, so a daemon exit orphans swiftc and its cache lock

prepareVideoValidator fires a detached compile with no signal or owner (compileSwiftSourceText called without timeoutMs, falling back to the 120s default). A daemon exit during warm-up orphans swiftc holding <executablePath>.lock; the next daemon's warm-up waits on that lock and can report a non-retriable video_validator_unavailable caused by a dead process.

Suggested change

Keep an owner for the background compile (a daemon-lifetime AbortController passed as signal, or an unref'd bound timeout), or state in CLI text/docs that a daemon exiting mid-warm-up leaves swiftc running.

Details

persona: javascript

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.


export async function waitForStableFile(
filePath: string,
Expand Down Expand Up @@ -107,7 +114,17 @@
const validatorPath = await waitForValidatorCompile(
host,
deadline?.remainingMs() ?? VALIDATOR_COMPILE_WAIT_MS,
);
).catch((error: unknown) => {
if (
error instanceof AppError &&
error.cause instanceof AppError &&
error.cause.code === 'TOOL_MISSING'
)
return undefined;
throw error;
});
// Without compiler-launch tooling, MP4 completion relies on the container check.
if (!validatorPath) return true;
const runTimeoutMs = Math.min(
VALIDATOR_RUN_TIMEOUT_MS,
deadline?.remainingMs() ?? VALIDATOR_RUN_TIMEOUT_MS,
Expand All @@ -126,48 +143,22 @@
if (result.exitCode === 0) {
return true;
}
return isSwiftVideoValidatorUnavailable(result.stderr, result.stdout);
if (isSwiftVideoValidatorUnavailable(result.stderr, result.stdout)) {
throw unavailableVideoValidator(
new AppError('COMMAND_FAILED', result.stderr, execFailureDetails(result)),
);
}
Comment on lines +146 to +150

@apex-sentinel-staging apex-sentinel-staging Bot Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No longer raised on ab5a5e5: the code it pointed at changed.

🟡 Minor · Compiler stderr never reaches the validator-unavailable details, so the xcrun hint is dead

Validator writes no such module 'AVFoundation' on stderr; record stop returns video_validator_unavailable with the generic repair hint and no stderr in details. unavailableVideoValidator reads cause.details?.stderr to pick the hint, but the only caller throws new AppError('COMMAND_FAILED', result.stderr) with details undefined, so that branch is unreachable for a run failure. The unavailable.mp4 tests assert only reason and retriable, which is why coverage misses it.

Suggested change
if (isSwiftVideoValidatorUnavailable(result.stderr, result.stdout)) {
throw unavailableVideoValidator(new AppError('COMMAND_FAILED', result.stderr));
}
if (isSwiftVideoValidatorUnavailable(result.stderr, result.stdout)) {
throw unavailableVideoValidator(
new AppError('COMMAND_FAILED', result.stderr, {
stderr: result.stderr,
stdout: result.stdout,
exitCode: result.exitCode,
}),
);
}
Details

Also raised here

Also raised at this location by persona maintainability: Validator run failure loses stderr in details, so the AVFoundation hint is never used — When the compiled validator itself runs and reports no such module 'AVFoundation' on stderr, the user gets the generic "Repair the Swift toolchain or validator executable" hint and no details.stderr, even though the code just classified it as an AVFoundation/xcrun problem. Pass stderr/stdout/exitCode as details of the thrown AppError. This is also why none of the existing tests catch it — the unavailable.mp4 cases assert only reason and retriable, not hint or stderr, unlike the compiler-diagnostics case which does assert both.

Also raised at this location by persona senior-developer: Validator's own stderr is passed as the message, so the AVFoundation hint never fires — The cause is that line 147 puts the validator's stderr into the AppError message (new AppError('COMMAND_FAILED', result.stderr)) and not into details.stderr, so the classifier at lines 200-203, which only reads details?.stderr/details?.stdout, cannot see it and picks the other branch. The compile-failure path does populate details.stderr (the new preserves compiler stderr test covers it), which is why the hint is right there and wrong here. Pass the diagnostics as details so both paths classify identically.

persona: maintainability, senior-developer

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

return false;
} catch (error) {
if (isSwiftVideoValidatorError(error)) {
return true;
throw unavailableVideoValidator(error);
}
throw error;
}

Check warning on line 157 in packages/capture-kit/src/recording/video.ts

View check run for this annotation

apex-sentinel-staging / Apex Sentinel

minor: Unplayable recording is now reported as non-retriable validator-unavailable after one validator refusal

A corrupt MP4 (ftyp+moov present but unplayable) whose validator exits non-zero with an 'unavailable' string (e.g. stderr containing `xcrun: error: unable to find utility "swiftc"`) fails `record stop` with `retriable:false` / reason `video_validator_unavailable` instead of the retriable `RECORDING_OUTPUT_UNPLAYABLE_REASON`. `isPlayableVideo` now throws out of the `exitCode !== 0` branch (video.ts:146-151), and `finalizeScreenRecording` (platform-runtime-screen-recording-finalizer-host.ts:38-40) has no catch for it, so the unplayable-recording refusal is never reached.
Comment on lines +146 to 157

@apex-sentinel-staging apex-sentinel-staging Bot Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No longer raised on 823b8d1, with the code it pointed at unchanged: withdrawn, not fixed.

🟡 Minor · record stop fails on daemon hosts without xcrun, where it used to succeed

record stop for an MP4 recording on a host without xcrun (Linux, Windows, or macOS with only Command Line Tools) now fails with COMMAND_FAILED / details.reason: video_validator_unavailable, retriable: false; previously a missing validator meant "trust the container sniff" and the recording finalized. isPlayableVideo runs runCmd('xcrun', ...) to compile the validator, gets TOOL_MISSING, and throws instead of returning true as the predecessor's isSwiftVideoValidatorError branch did. The failure is cached per cache root, so no retry clears it without a daemon restart.

Suggested change

Decide whether a host with no Swift toolchain may still finalize recordings: if yes, treat TOOL_MISSING as "accept the container sniff" as before and reserve video_validator_unavailable for a toolchain that exists but fails; either way state the decision in website/docs/docs/commands.md.

Details

Also raised here

Also raised at this location by persona senior-developer: Missing xcrun now hard-fails Android/HarmonyOS record stop instead of finalizing — record stop on an Android or HarmonyOS recording fails with non-retriable video_validator_unavailable on any host without a working Swift toolchain — e.g. a Linux daemon, the common setup for ADB recording. xcrun is absent there, so the compile throws TOOL_MISSING, isSwiftVideoValidatorError matches it, and isPlayableVideo now throws where it previously returned true; the Android pull path (src/platform-runtime-screen-recording-android-host.ts:112) and the finalizer propagate that error, so the MP4 the device already produced is never reported. The hint then tells the operator to install Swift with AVFoundation on the host, which is not how an Android recording is verified. The error should stay scoped to hosts that actually need the MP4 validator, or the WebM/absent-toolchain case must keep its old verdict.

persona: javascript, senior-developer

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

Comment on lines +146 to 157

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🆕 🟡 Minor · Unplayable recording is now reported as non-retriable validator-unavailable after one validator refusal

A corrupt MP4 (ftyp+moov present but unplayable) whose validator exits non-zero with an 'unavailable' string (e.g. stderr containing xcrun: error: unable to find utility "swiftc") fails record stop with retriable:false / reason video_validator_unavailable instead of the retriable RECORDING_OUTPUT_UNPLAYABLE_REASON. isPlayableVideo now throws out of the exitCode !== 0 branch (video.ts:146-151), and finalizeScreenRecording (platform-runtime-screen-recording-finalizer-host.ts:38-40) has no catch for it, so the unplayable-recording refusal is never reached.

Suggested change

Either classify a non-zero validator exit as an unplayable recording and keep the throw for launch-time failures only, or catch video_validator_unavailable in finalizeScreenRecording and re-raise it with retriable:true.

Details

persona: javascript

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

}

function joinValidatorCompile(host: HostEnvironment, cacheRoot: string): SharedValidatorCompile {
const existing = videoValidatorCompiles.get(cacheRoot);
if (existing) return existing;
const cancel = new AbortController();
const shared: SharedValidatorCompile = {
compiled: compileSwiftSourceText({
host,
source: VIDEO_VALIDATION_SCRIPT,
cacheName: 'video-validator',
signal: cancel.signal,
}),
cancel,
waiters: 0,
settled: false,
};
videoValidatorCompiles.set(cacheRoot, shared);
shared.compiled.then(
() => {
shared.settled = true;
},
() => {
shared.settled = true;
if (videoValidatorCompiles.get(cacheRoot) === shared)
videoValidatorCompiles.delete(cacheRoot);
},
);
return shared;
}

async function waitForValidatorCompile(host: HostEnvironment, waitMs: number): Promise<string> {
const cacheRoot = getSwiftCacheRoot(host.env);
const shared = joinValidatorCompile(host, cacheRoot);
shared.waiters += 1;
const compiled = prepareVideoValidator(host);
let timer: NodeJS.Timeout | undefined;
const expired = new Promise<never>((_, reject) => {
timer = setTimeout(
Expand All @@ -181,28 +172,42 @@
timeoutMs: waitMs,
retriable: true,
hint:
'Run record stop again: the Swift modules this compile already built stay ' +
'cached, so the next compile finishes sooner, and the recording keeps its ' +
'evidence either way.',
'Run record stop again: the validator compile continues in the background, ' +
'and the recording keeps its evidence.',
},
),
),
waitMs,
);
});
try {
return await Promise.race([shared.compiled, expired]);
return await Promise.race([compiled, expired]);
} finally {
clearTimeout(timer);
shared.waiters -= 1;
if (shared.waiters === 0 && !shared.settled) {
if (videoValidatorCompiles.get(cacheRoot) === shared)
videoValidatorCompiles.delete(cacheRoot);
shared.cancel.abort();
}
}
}

function unavailableVideoValidator(cause: unknown): AppError {
if (cause instanceof AppError && cause.details?.reason === 'video_validator_unavailable')
return cause;
const details = cause instanceof AppError ? cause.details : undefined;
return new AppError(
'COMMAND_FAILED',
'The video validator is unavailable; MP4 playability could not be verified.',
{
...details,
reason: 'video_validator_unavailable',
retriable: false,
hint:
details?.hint ??
(isSwiftVideoValidatorUnavailable(details?.stderr ?? '', details?.stdout ?? '')
Comment thread
thymikee marked this conversation as resolved.
Resolved
? 'Ensure Swift and AVFoundation are available on the daemon host, then retry record stop.'
: 'Repair the Swift toolchain or validator executable on the daemon host, then retry record stop.'),
},
cause,
);
}

Check warning on line 209 in packages/capture-kit/src/recording/video.ts

View check run for this annotation

apex-sentinel-staging / Apex Sentinel

minor: video\_validator\_unavailable is reported non-retriable while docs and hint say to retry record stop

A compile failure (e.g. `xcrun` can't find `swiftc` due to a wrong `DEVELOPER_DIR`, or the 120s timeout) returns `details.retriable:false`, but the docs bullet added here and the generated hint both say "Repair the reported failure and retry record stop", and `prepareVideoValidator` evicts the rejected compile so the next attempt re-runs it. A client honouring `retriable:false` will therefore not retry a transient failure the code path supports retrying. Cause: `unavailableVideoValidator` hardcodes `retriable:false` for every cause without distinguishing transient compile failures from a permanently broken validator executable. ### Also raised here Also raised at this location by persona `maintainability`: A retriable compile failure is reported as non-retriable, contradicting the retry guidance — A transient compile failure (swiftc killed by OOM, a cache-lock contention failure) is reported as `retriable: false`, yet the same change evicts the rejected compile so the next attempt recompiles, and the new cliDetail and commands.md both say "Repair the reported failure and retry `record stop`; a failed compile is retried on the next start or stop". `unavailableVideoValidator` hard-codes `retriable: false` for every cause, so a client that honours `details.retriable` refuses the retry the docs instruct, and the flag contradicts the author's own guidance for the compile-failure case. Carry the cause's retriability through (or mark compile failures retriable) instead of forcing false.
Comment on lines +194 to +209

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🆕 🟡 Minor · video_validator_unavailable is reported non-retriable while docs and hint say to retry record stop

A compile failure (e.g. xcrun can't find swiftc due to a wrong DEVELOPER_DIR, or the 120s timeout) returns details.retriable:false, but the docs bullet added here and the generated hint both say "Repair the reported failure and retry record stop", and prepareVideoValidator evicts the rejected compile so the next attempt re-runs it. A client honouring retriable:false will therefore not retry a transient failure the code path supports retrying. Cause: unavailableVideoValidator hardcodes retriable:false for every cause without distinguishing transient compile failures from a permanently broken validator executable.

Suggested change

Derive retriable from the cause—true for a compile failure or timeout that the next attempt re-runs, false only for a validator executable that cannot work—and make the docs and hint agree with that signal.

Details

Also raised here

Also raised at this location by persona maintainability: A retriable compile failure is reported as non-retriable, contradicting the retry guidance — A transient compile failure (swiftc killed by OOM, a cache-lock contention failure) is reported as retriable: false, yet the same change evicts the rejected compile so the next attempt recompiles, and the new cliDetail and commands.md both say "Repair the reported failure and retry record stop; a failed compile is retried on the next start or stop". unavailableVideoValidator hard-codes retriable: false for every cause, so a client that honours details.retriable refuses the retry the docs instruct, and the flag contradicts the author's own guidance for the compile-failure case. Carry the cause's retriability through (or mark compile failures retriable) instead of forcing false.

persona: javascript, senior-developer, maintainability

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if this is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.


function isSwiftVideoValidatorError(error: unknown): boolean {
if (!(error instanceof AppError)) {
return false;
Expand Down Expand Up @@ -240,7 +245,7 @@

function isSwiftVideoValidatorUnavailable(stderr: string, stdout: string): boolean {
const combined = `${stderr}\n${stdout}`;
return /\b(no such module ['"]AVFoundation['"]|unable to find utility ["']swiftc?["']|xcrun: error: unable to find utility ["']swiftc?["'])\b/i.test(
return /\b(no such module ['"]AVFoundation['"]|unable to find utility ["']swiftc?["']|xcrun: error: unable to find utility ["']swiftc?["'])/i.test(
combined,
);
}
Expand Down
Loading
Loading