Repository navigation
fix(record): bound a cold video-validator compile by the stop's finalize budget - #4150
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
8e2f9fe to
08f3f3d
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 4 new · 1 still open · 5 no longer raised 5 finding(s): 3 minor, 2 nit. Verdict: approve. 🟡 Minor (3)
🔵 Nit (2)
Reply under an inline comment with Earlier reviewsReview of
|
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/overlay.ts:116 |
🆕 Post-export playability check is now clamped to the 5 s exit grace, so a slow validator discards a good composite |
packages/capture-kit/src/recording/video.ts:193 |
Concurrent validator-compile waiters lose the shared compile and reap the in-flight swiftc after the first timeout |
packages/capture-kit/src/recording/video.ts:194 |
🆕 Abandoning a shared compile now also stops the compile that previously completed and was cached |
src/platform-runtime-screen-recording-finalizer-host.ts:36 |
A budget spent before the validator runs is reported to the user as an unplayable recording |
🔵 Nit (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:105 |
🆕 isPlayableVideo catch clause gains a body with no comment; the 30s/10s split lives only in tests |
packages/capture-kit/src/recording/video.ts:193 |
Validator-compile cancellation is not recorded as a distinct command failure reason |
Review of afefe6b0 · 02.11.2026 20:18 UTC
Apex Sentinel
Verdict: request_changes
Since the last review: 7 new · 2 still open · 3 no longer raised
9 finding(s): 1 major, 4 minor, 4 nit. Verdict: request_changes.
Required fixes
- waitForPlayableVideo now rejects with the compile-timeout error, skipping every remaining poll and bypassing the
unplayableRecordinghint
🟠 Major (1)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:230 |
🆕 waitForPlayableVideo now rejects with the compile-timeout error, skipping every remaining poll and bypassing the unplayableRecording hint |
🟡 Minor (4)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:106 |
🆕 A validator run that hits its own bound throws for every caller with 10 s+ of budget left |
packages/capture-kit/src/recording/video.ts:110 |
Validator run timeout mapped to false hides a genuinely broken video when the deadline had only milliseconds left |
packages/capture-kit/src/recording/video.ts:138 |
Shared compile drops the 30s timeoutMs, so the Swift-cache lock is now held on a 120s bound |
packages/capture-kit/src/recording/video.ts:172 |
🆕 Unreferenced VALIDATOR_COMPILE_WAIT_MS in the error the caller receives |
🔵 Nit (4)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/swift-cache.ts:90 |
🆕 compileSwiftSourceFile (the overlay export compile) gains no cancel path, so the SIGINT cleanup this PR adds stops at the validator |
packages/capture-kit/src/recording/swift-cache.ts:177 |
🆕 Swift cache lock acquire timeout still equals the compile timeout, so a canceled wait can make another process's healthy compile time this one out |
packages/capture-kit/src/recording/video.ts:130 |
🆕 A validator compile interrupted by another caller now reads as 'playable' instead of failing |
packages/capture-kit/src/recording/video.ts:154 |
🆕 Shared-compile bookkeeping relies on a settled flag that races the last-waiter abort, so a completed compile can be dropped from the map and re-joined on the next call |
Review of 009d67cd · 02.11.2026 19:50 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 2 still open · 3 no longer raised
5 finding(s): 1 minor, 4 nit. Verdict: approve.
🟡 Minor (1)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/overlay.ts:86 |
🆕 The overlay's own compile lost its 30 s bound and inherits the finalization budget, so it can overrun the 90 s daemon envelope |
🔵 Nit (4)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:115 |
🆕 A validator run SIGKILLed at a clamped deadline reads as a Swift-validator failure and reports an unplayable recording as playable |
packages/capture-kit/src/recording/video.ts:137 |
🆕 Shared validator compile drops the compiler's own timeout bound |
packages/capture-kit/src/recording/overlay.ts:120 |
Overlay reports a spent budget as an export overrun after the playability wait, masking the compile-timeout error's details |
packages/capture-kit/src/recording/video.ts:93 |
webm skips the new spent-budget guard, unlike mp4 |
Review of 43963597 · 02.11.2026 19:18 UTC
Apex Sentinel
Verdict: approve
Since the last review: 3 new · 2 still open · 13 no longer raised
5 finding(s): 2 minor, 3 nit. Verdict: approve.
🟡 Minor (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:103 |
🆕 Android's playable probe now waits up to 30 s for a validator compile where it used to get a cached path |
packages/capture-kit/src/recording/video.ts:193 |
A compile aborted for want of waiters can fail every validator check in the next 2 s, and the overlay treats that as an unplayable video |
🔵 Nit (3)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/overlay.ts:120 |
🆕 Overlay discards a composited video when a validator check throws, instead of using the deadline verdict the wait returns |
packages/capture-kit/src/recording/video.ts:103 |
🆕 Validator run timeout is clamped against the caller deadline, not just the 10 s default |
packages/capture-kit/src/recording/video.ts:152 |
The cache-root key is computed twice, once to insert and once to clean up |
Review of 68380584 · time not recorded
Apex Sentinel
Verdict: request_changes
Since the last review: 4 new · 2 still open · 9 no longer raised
6 finding(s): 1 major, 2 minor, 3 nit. Verdict: request_changes. reached max rounds (1).
Required fixes
- Decide between: Deadline-clamped validator run rejects and escapes as an exec timeout instead of reading as not playable / waitForPlayableVideo's new deadline makes the overlay's post-export verification fail spuriously on slow exports
🟠 Major (1)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:108 |
🆕 Deadline-clamped validator run rejects and escapes as an exec timeout instead of reading as not playable |
🟡 Minor (2)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:136 |
🆕 Cold validator compile is no longer bounded by swiftc's own timeout; the wait is bounded only while a caller waits |
packages/capture-kit/src/recording/video.ts:224 |
waitForPlayableVideo's new deadline makes the overlay's post-export verification fail spuriously on slow exports |
🔵 Nit (3)
| Where | Finding |
|---|---|
packages/capture-kit/src/recording/video.ts:212 |
waitForPlayableVideo now runs attempts + 1 validator passes, doubling the validator run for an unplayable file |
packages/capture-kit/src/recording/video.ts:103 |
🆕 Run timeout clamps the deadline twice in one nested expression |
packages/capture-kit/src/recording/video.ts:143 |
🆕 shared.settled is set from a .then(...) handler on shared.compiled, and waitForValidatorCompile's finally r… |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on 08f3f3d. The verdict and the full list are in the summary comment.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on 3a127d3. The verdict and the full list are in the summary comment.
There was a problem hiding this comment.
Apex Sentinel: 4 new findings and 1 moved to a new line on 6838058. The verdict and the full list are in the summary comment.
|
The fix reads well at 6838058, but the new cancel path has no live run yet, so it is not ready to merge. CI shows no failing job; the only non-passing check is the review bot's pending request-changes status. The only live measurement in the PR table is from the first commit. The head now uses a shared compile where the last waiter aborts swiftc (video.ts:130), and Not blocking, and you can take or leave these: (1) if the compile ends with a few ms of budget left, the clamped validator run in video.ts:105 times out and Of the open inline threads, the two on the clamped validator-run timeout (r4437312410, r4437338234) still apply, and three lower-priority threads (comment wording, the nested min) still apply. These do not apply at the head, so please resolve them: r4437243626, r4437312427 and r4437338242 (every waiter is bounded and the last aborts swiftc); r4437243630 (a spent budget returns an overlay warning); r4437243632 (the error carries retriable and a hint); r4437243637 (that code no longer exists); r4437312420 (the typed timeout is intended); r4437312423 (base ran the same passes); r4437312430 (the canceled rejection is consumed internally); r4437338247 (base already checked the deadline); r4437338255 (settled is set before any waiter finishes). This review is from reading the diff. I did not run the tests or the simulator repro. I could not tie the 90 s stop envelope or the 80 s worst case to a constant on the record stop route, or confirm that built Swift modules stay cached after an abort, as the retry hint says. Before merge, please post the cold-cache live run above. |
6838058 to
4396359
Compare
|
Rebased on main and addressed the review. Head is now What changed
Live runSetup: iPhone 17e simulator (a) A cold (b) Forced abandon at the head, from a throwaway script that is not committed. It calls Before the SIGINT fix, the same steps left a Next stop on the same cache, at the head: the stop compiled again and passed. The compile was still over 30 s because the host load peaked near 700. The session was closed and the daemon was stopped afterwards. Review threadsI resolved the 11 threads the review says do not apply, and the 4 threads this push fixes: the two clamped-run threads, the comment wording and the nested min. Validation
|
There was a problem hiding this comment.
Apex Sentinel: 3 new findings and 2 moved to a new line on 4396359. The verdict and the full list are in the summary comment.
| // AVFoundation is the MP4 semantic validator. It does not reliably load WebM on supported | ||
| // macOS hosts, so WebM completion is established by its EBML document type + Segment marker. | ||
| if (container === 'webm') return true; | ||
| try { |
There was a problem hiding this comment.
No longer raised on 009d67c.
🆕 🟡 Minor · Android's playable probe now waits up to 30 s for a validator compile where it used to get a cached path
isPlayableVideo gained an optional deadline, and every caller that does not pass one now falls back to deadline?.remainingMs() ?? VALIDATOR_COMPILE_WAIT_MS for the compile wait (video.ts:105-108). The new videoValidatorCompiles map also no longer drops a successful compile the way the old map dropped a failed one: it keeps the entry forever, so any transient compile failure permanently costs every deadline-less caller a 30 s stall in addition to its real work (the next caller starts a fresh compile and waits the full wait again). Before this diff, the same path could not wait longer than the compile's own timeoutMs: 30_000 and returned as soon as the previous attempt's promise settled.
Suggested change
Note in the isPlayableVideo doc comment that a caller without a deadline waits a fixed 30 s for a cold compile, and consider passing a short deadline from the Android pull path so the probe stays bounded.
Details
The caller that feels it is createAndroidScreenRecordingTransport (src/platform-runtime-screen-recording-android-host.ts:112,116), unchanged by this PR: its playable: result.exitCode === 0 && (await isPlayableVideo(outputPath, host)) inside pullPlayable is the one place where the probe runs for a file it has just pulled and that may not be complete yet, so a cold cache plus a validator failure turns a quick playable: false into 30 s + 120 s before it answers.
Also raised here
Also raised at this location by javascript: isPlayableVideo's new deadline semantics are not stated for the callers that pass no deadline, which now wait a fixed 30 s for a cold compile — The new doc comment on isPlayableVideo (video.ts:88-92) describes only what happens 'With a deadline'. The two production callers that pass none — createAndroidScreenRecordingTransport (src/platform-runtime-screen-recording-android-host.ts:112) and test/integration/live-device-e2e/assertions.ts:92 — are exactly the ones whose worst case this diff changed: they now wait VALIDATOR_COMPILE_WAIT_MS (30 s) plus the compile's own params.timeoutMs ?? 120_000 default in ensureSwiftExecutable (swift-cache.ts:179), because joinValidatorCompile passes no timeoutMs at all. State in that comment what a deadline-less caller is guaranteed, or name the constant it should pass instead of relying on it.
Also raised at this location by senior-developer: The compile now runs under a silent 120 s internal bound because timeoutMs is no longer passed — joinValidatorCompile (video.ts:143-148) calls compileSwiftSourceText without timeoutMs, while the removed predecessor passed timeoutMs: 30_000. That field is the only bound ensureSwiftExecutable has on the xcrun swiftc run: const timeoutMs = params.timeoutMs ?? 120_000 (swift-cache.ts:179) arms both the lock acquire and the run. The waitMs used by waitForValidatorCompile is a wait bound only — it never reaches the child. So a lone caller without a deadline (Android pullPlayable in src/platform-runtime-screen-recording-android-host.ts:112, test/integration/live-device-e2e/assertions.ts:92) gives up after 30 s while swiftc keeps compiling up to 120 s, holding the executable lock so every concurrent compile of that key times out; and a caller with a Deadline whose isExpired() goes stale (the timer is armed from remainingMs() and nothing re-checks it, video.ts:172-192) can keep a compile alive far past its budget. …
…and 1 more (javascript), left out to fit the comment.
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.
| 'COMMAND_FAILED', | ||
| 'The composited video was not verified within the budget', | ||
| ); | ||
| if (!(await waitForPlayableVideo(outputPath, { host, deadline }))) { |
There was a problem hiding this comment.
🆕 🟡 Minor · exportOverran is not set when the composited video fails playability verification before the budget expires
The old code set exportOverran = true whenever the verification step found the deadline expired. The new branch sets exportOverran = deadline.isExpired(), so when waitForPlayableVideo returns false with budget still on the clock — which is now a reachable state, because a clamped runTimeoutMs that times out, or a validator that exits nonzero, both read as not playable instead of throwing — the operator gets the bare commandDescription ("Failed to add touch overlays to the recording") with no budget hint. Worse, the details still carry the copied reason: 'video_validator_compile_timeout', retriable: true and the "Run record stop again" hint from the wrapped cause, so a run that had most of the 70 s left is described both as a plain overlay failure and as a compile timeout. Reproduce: overlay export succeeds but the composited file's validator run times out within a clamped runTimeoutMs while the deadline still has seconds left.
Suggested change
Pick one description for the not-verified path (either set exportOverran for it or drop the copied compile-timeout details) so the message and the details describe the same failure.
Details
Also raised here
Also raised at this location by javascript: Overlay's budget-overrun message now depends on the wait's false verdict coinciding with isExpired() — exportProcessedVideo replaced the two-step check (await waitForPlayableVideo(...) then deadline.isExpired()) with exportOverran = deadline.isExpired() read only after the wait resolves false (overlay.ts:120-122). Because waitForPlayableVideo now also returns false for reasons other than the budget — attempts exhausted (12 polls of 150 ms at the default, video.ts:228-236), or the validator run timing out inside isPlayableVideo (video.ts:117-125) — the exportOverran flag silently relies on 'expired' and 'not playable' being the same event. When they diverge (a composited export that fails 12 validator polls with budget left, or a validator run killed by runTimeoutMs), the user loses the the export did not finish within its 70000ms budget; a lower --fps renders faster advice and gets only the plain command description, even though the budget is what made the run too short. Say which condition sets the flag instead of inferring it from the clock.
Also raised at this location by senior-developer: New video_validator_compile_timeout reason is thrown from record stop outside the recording-runtime reason contract — waitForValidatorCompile rejects with a new reason: 'video_validator_compile_timeout' (video.ts:176-188), and finalizeScreenRecording lets it propagate: the earlier unconditional waitForPlayableVideo + isPlayableVideo pair that always threw unplayableRecording() with the contract-owned RECORDING_OUTPUT_UNPLAYABLE_REASON is replaced by one if (!(await waitForPlayableVideo(...))) (src/platform-runtime-screen-recording-finalizer-host.ts:38-40). So a cold compile that outlasts the 70 s finalize budget now fails record stop with a reason string that is defined inline in capture-kit and not part of the @agent-device/contracts/screen-recording-runtime vocabulary the other stop failures use; a client or doc that branches on the recording reasons sees a value it has never seen, and the same stop can fail with either reason depending on whether the compile or the file is at fault. …
persona: senior-developer, 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.
|
The code looks good at 4396359, and the earlier finding from the 6838058 review (#4150 (comment)) is settled. The finalizer's budget now bounds the cold validator compile, a run the deadline cuts short reads as not playable, and the last caller to stop waiting aborts Not blocking, take or leave: the clamped-run regression in video.test.ts:194 uses The earlier inline threads are fixed at this head or benign as written, including the two P2 threads on the deadline-clamped run (#4150 (comment), #4150 (comment)). The newer P3 and P4 threads do not apply: the sole no-deadline waiter leaves after 30 s and aborts This review covers 4396359. The newer head 009d67c only keys the shared validator compile once per wait in video.ts, which also settles the duplicated Swift root nit, so the points above still apply. |
There was a problem hiding this comment.
Apex Sentinel: 4 findings moved to a new line on 009d67c. The verdict and the full list are in the summary comment.
| ); | ||
| // A budget with no time left reads as not playable; `runCmd` would read a timeout of 0 as none. | ||
| if (runTimeoutMs <= 0) return false; | ||
| const result = await runCmd(validatorPath, [filePath], { |
There was a problem hiding this comment.
No longer raised on afefe6b.
🆕 🔵 Nit · A validator run SIGKILLed at a clamped deadline reads as a Swift-validator failure and reports an unplayable recording as playable
isPlayableVideo now passes a deadline-clamped timeoutMs to the validator run (video.ts:115-118), and the only thing separating "the run was killed at the caller's deadline" from "the run hit the validator's own 10 s bound" is the caller-side runTimeoutMs < VALIDATOR_RUN_TIMEOUT_MS test at video.ts:121. packages/host-kit/src/internal/exec.ts makes that test incomplete in two ways, and both are newly reachable because a short timeoutMs is only possible after this diff (the predecessor always passed a fixed 10_000).
Suggested change
Stop inferring who cut the validator run short from runTimeoutMs < VALIDATOR_RUN_TIMEOUT_MS. Classify the kill from exec's own structured state: either expose an abort/deadline reason on the error exec raises (the timedOut: true shape at exec.ts:429-433 already exists and settle() at exec.ts:246-259 already knows abort.didAbort/didTimeout), or hand the validator run an AbortSignal derived from the deadline and treat isRequestCanceledError as "not playable". Also do not feed a killed validator child's partial stderr into isSwiftVideoValidatorUnavailable: run that fallback only for a run that exited on its own.
Details
-
createTimeoutError(exec.ts:651-670) is armed only insettle()when the child alreadyclosed (exec.ts:256), and the default stop is SIGKILL —SWIFTC_STOPis passed only forxcrun swiftcin swift-cache.ts:204, not for the validator run. So a validator run that is SIGKILLed at e.g. 400 ms and whose stdio pipes stay open (any grandchild holding them, or a stream that does not close on SIGKILL) never reachessettle();isCommandTimeoutErroris never consulted and theAppError('COMMAND_FAILED', 'The command was aborted by a deadline', { timedOut: true })from exec.ts:429-433 propagates through the catch at video.ts:129 and out ofisPlayableVideo.waitForPlayableVideopropagates it too, sofinalizeScreenRecording(src/platform-runtime-screen-recording-finalizer-host.ts:38) fails the wholerecord stopwith a bareCOMMAND_FAILEDcarrying noreason, instead of the documented and tested "reads as not playable" behavior (video.test.ts: 'a validator run the caller deadline cuts short reads as not playable' asserts only theclose-on-kill path, because it mocksrunCmd). A retried stop then succeeds, which is exactly the symptom the PR set out to remove. -
The same abort error carries
details: { timedOut: true }and notimeoutMs, soisCommandTimeoutError(exec.ts:677-683, which checks onlytypeof error.details?.timeoutMs === 'number') answers false for it as well. -
When the kill does settle,
createTimeoutErrorputs the killed child's capturedstderr/stdoutintodetails(exec.ts:662-669). For SIGKILL there is no flush, so a truncated AVFoundation/ObjC diagnostic is exactly what lands there. The video under test was written by the same AVFoundation helper that has now died, andisSwiftVideoValidatorUnavailable(video.ts:240-245) matchesunable to find utility "swiftc"— if that substring survives in the killed child's partial stderr,isPlayableVideoreturns true (video.ts:128) for an unplayable composited video, andoverlay.ts:120-124renames the corrupt output over the original recording. The 10 s-bound case this regex was written for flushes its stderr and exits normally, so this mixing of killed-child output into the fallback is new to this diff.
The distinguishing state already exists in exec: !abortSignal.aborted && !didTimeout. Classifying from the structured error rather than the caller's own timeout arithmetic is what AGENTS.md asks for ("Use typed reasons and details for behavior. Do not match error text.").
Also raised here
…and 1 more (senior-developer), left out to fit the comment.
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.
| let compiled = videoValidatorExecutablePaths.get(cacheRoot); | ||
| if (!compiled) { | ||
| compiled = compileSwiftSourceText({ | ||
| function joinValidatorCompile(host: HostEnvironment, cacheRoot: string): SharedValidatorCompile { |
There was a problem hiding this comment.
No longer raised on cdf9516.
🟡 Minor · Shared compile drops the 30s timeoutMs, so the Swift-cache lock is now held on a 120s bound
getVideoValidatorExecutablePath compiled with timeoutMs: 30_000, and ensureSwiftExecutable uses that one value for both the lock acquire and the xcrun swiftc run (swift-cache.ts:179-181,201-206). joinValidatorCompile passes no timeoutMs, so every validator compile now runs with the 120s default: a process that merely waits for the <executable>.lock held by another daemon's compile tolerates up to 120s of lock contention where it previously gave up after 30s, and the compile itself may run 4x longer before the run's own bound fires (bounded only by cancellation when the last waiter leaves).
| function joinValidatorCompile(host: HostEnvironment, cacheRoot: string): SharedValidatorCompile { | |
| compiled: compileSwiftSourceText({ | |
| host, | |
| source: VIDEO_VALIDATION_SCRIPT, | |
| cacheName: 'video-validator', | |
| timeoutMs: VALIDATOR_COMPILE_WAIT_MS, | |
| signal: cancel.signal, | |
| }), |
Suggested change
Pass an explicit timeoutMs to the shared compileSwiftSourceText call (or split lock timeout from compile timeout in ensureSwiftExecutable) so the validator's lock wait keeps a bound close to the 30s it had before.
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.
|
Follow-up on the latest bot round. Head is now Changed
Not changed, after verification
|
There was a problem hiding this comment.
Apex Sentinel: 8 findings moved to a new line on afefe6b. The verdict and the full list are in the summary comment.
| host, | ||
| deadline?.remainingMs() ?? VALIDATOR_COMPILE_WAIT_MS, | ||
| ); | ||
| const runTimeoutMs = Math.min( |
There was a problem hiding this comment.
No longer raised on cdf9516.
🟡 Minor · Validator run timeout mapped to false hides a genuinely broken video when the deadline had only milliseconds left
packages/capture-kit/src/recording/video.ts:110-125: runTimeoutMs = Math.min(VALIDATOR_RUN_TIMEOUT_MS, deadline?.remainingMs()), and any timeout from that run becomes undefined → return false whenever runTimeoutMs < VALIDATOR_RUN_TIMEOUT_MS. remainingMs() is exact and can be 1–50ms with a live deadline, so the validator is killed almost at once and the answer is "not playable". waitForPlayableVideo then reports false, and src/platform-runtime-screen-recording-finalizer-host.ts:38-40 turns that into unplayableRecording ("recording was not finalized into a playable video", reason RECORDING_OUTPUT_UNPLAYABLE_REASON) for a file that was most likely fine — the finalizer's own test 'a recording confirmed playable as the budget runs out is not refused by a second check' states that a spent budget reads as not playable, so this mislabelled path is reachable at exactly that boundary. A run cut short with almost no budget left is a no verdict (like the compile-timeout error above it, which stays a retriable error), not evidence the video is broken: the error already carries the killed validator's exit, so the two cases could be told apart.
Details
persona: 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.
|
Final round. Head is now Changed (r4447277445): every validator run timeout now reads as not playable. The deadline-clamp condition is gone. Before this change, a run that hit its own 10 s bound threw when the caller had 10 s or more of budget left, or had no deadline. A run the deadline cut short already returned Left as is: Live run at
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 1 new finding and 5 moved to a new line on cdf9516. The verdict and the full list are in the summary comment.
| @@ -117,13 +117,9 @@ async function exportProcessedVideo(params: { | |||
| exportOverran = deadline.remainingMs() <= HELPER_EXIT_GRACE_MS; | |||
There was a problem hiding this comment.
No longer raised on a1af133.
🆕 🟡 Minor · Post-export playability check is now clamped to the 5 s exit grace, so a slow validator discards a good composite
Passing deadline into waitForPlayableVideo (overlay.ts:120) now propagates it into isPlayableVideo, where the validator run is clamped to Math.min(10_000, deadline.remainingMs()) and a run that times out reads as not playable (video.ts:111-125). By construction the helper exits with about HELPER_EXIT_GRACE_MS = 5 000 ms left (overlay.ts:94-95, 114), so the very first validation attempt gets ≤5 s instead of the fixed 10 s the predecessor gave it, and every later poll gets less. On a loaded host where AVFoundation needs ~5.5 s to load the composited file, the attempt times out → false → 150 ms sleep → the deadline expires → waitForPlayableVideo returns false → exportOverran = deadline.isExpired() is true → the verified-composite rename at overlay.ts:124 never happens and the user gets "the export did not finish within its 70000ms budget" with the touch overlay silently dropped, even though the composite was fine. The predecessor ran the same check with the full 10 s. Either give the post-export check its own floor (e.g. Math.max(1, deadline.remainingMs()) with a reserved validation slice inside HELPER_EXIT_GRACE_MS), or document that the exit grace must cover the validator run.
Suggested change
Reserve part of HELPER_EXIT_GRACE_MS for the post-export playability check (or keep that one check on the fixed VALIDATOR_RUN_TIMEOUT_MS) so a composite that is playable but slow to validate is not discarded at the end of the budget.
Details
persona: 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.
| } catch (error) { | ||
| videoValidatorExecutablePaths.delete(cacheRoot); | ||
| throw error; | ||
| return await Promise.race([shared.compiled, expired]); |
There was a problem hiding this comment.
No longer raised on a1af133.
🆕 🟡 Minor · Abandoning a shared compile now also stops the compile that previously completed and was cached
video.ts#getVideoValidatorExecutablePath used to resolve/publish whatever compileSwiftSourceText produced, and a failed or timed-out compile was never killed — it kept running and filled the Swift module cache (which is exactly what the new hint claims: "the Swift modules this compile already built stay cached"). The new waitForValidatorCompile finally-block calls shared.cancel.abort() as soon as the last waiter leaves, which aborts runCmd for this process's own compile too and SIGINTs swiftc mid-run (swift-cache.ts:203-204, SWIFTC_STOP). Concrete case: two record stop calls overlap and the second has a small remaining finalize budget, or a caller polls and gives up — the give-up kills the compile that would have finished and been reused, so the next stop recompiles from scratch and the retry the hint promises takes longer instead of sooner. Please gate the abort on lock ownership (only stop waiting for another process's compile; leave a compile this process owns running), or say in the comment/hint that abandoning throws away the partial work.
Suggested change
Do not cancel a compile this process owns when the last waiter gives up: only stop waiting for another process's compile, or record in the comment and the error hint that giving up discards the in-flight compile so the hint's caching claim stays true.
Details
persona: 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.
| input: Parameters<ScreenRecordingRuntimeHost['finalize']['complete']>[0], | ||
| ) { | ||
| const overlayDeadline = startRecordingOverlayBudget(); | ||
| const finalizeDeadline = startRecordingOverlayBudget(); |
There was a problem hiding this comment.
No longer raised on a1af133.
🟡 Minor · A budget spent before the validator runs is reported to the user as an unplayable recording
In the finalizer, a false from waitForPlayableVideo always throws unplayableRecording(...) (src/platform-runtime-screen-recording-finalizer-host.ts:38-40), whose message is "recording was not finalized into a playable video". With the deadline now threaded into isPlayableVideo, false is also what a never-attempted validation returns: deadline?.isExpired() (video.ts:106) or runTimeoutMs <= 0 (video.ts:116) short-circuit before the validator is spawned at all — that is the new "spent budget reads as not playable" contract asserted at video.test.ts:158-167. So a perfectly playable recording whose 70 s budget was spent by waitForStableFile plus a slow-but-successful compile and poll is refused with a message and hint ("Run record stop again: a recorder that is still finalizing its file…", lines 85-88) that point the user at the recorder rather than at the host load. The compile-timeout case was given its own retriable error (video.ts:176-188); the spent-budget-before-run case, which is the same host-load condition, was not.
Suggested change
Distinguish "the budget left no time to validate" from "the validator refused the file" — e.g. have the wait report the reason it stopped (a distinct retriable reason such as video_validator_budget_exhausted) so the user-facing hint names host load / retry rather than a recorder that is still finalizing.
Details
persona: 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.
| // AVFoundation is the MP4 semantic validator. It does not reliably load WebM on supported | ||
| // macOS hosts, so WebM completion is established by its EBML document type + Segment marker. | ||
| if (container === 'webm') return true; | ||
| try { |
There was a problem hiding this comment.
No longer raised on a1af133.
🆕 🔵 Nit · isPlayableVideo catch clause gains a body with no comment; the 30s/10s split lives only in tests
Two things a reader has to reconstruct here. (1) const runTimeoutMs = Math.min(...) (line 108) and the separate if (runTimeoutMs <= 0) return false; (line 111) look redundant — the guard is only needed because Math.min can return 0 and runCmd reads timeoutMs: 0 as "no timeout". The explanation sits above the later runCmd call (line 113), so the non-obvious line itself is bare. (2) The two new constants are documented as caller-side bounds (VALIDATOR_COMPILE_WAIT_MS comment mentions "waits … until that deadline instead") but nothing says that the compile itself is now launched with no timeoutMs at all, so the real wall-clock bound on swiftc for a deadline-less caller is ensureSwiftExecutable's 120s default rather than 30s; that split is asserted only in video.test.ts (expect(compile.timeoutMs).toBeUndefined()). A short comment on each line would keep the next change from "simplifying" the guard away or re-adding a timeoutMs.
Suggested change
Add a one-line comment on the runTimeoutMs computation saying Math.min can yield 0 and runCmd treats 0 as no timeout, and note on the new constants that the compile is started without timeoutMs so the caller wait, not a compile timeout, is the bound.
Details
persona: 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.
| waitMs, | ||
| ); | ||
| }); | ||
| try { |
There was a problem hiding this comment.
No longer raised on a1af133.
🔵 Nit · Validator-compile cancellation is not recorded as a distinct command failure reason
waitForValidatorCompile aborts its AbortController when the last waiter leaves (video.ts:201), and the resulting rejection of compileSwiftSourceText surfaces as the generic request-canceled error. The PR introduces a distinct, retriable reason for the sibling outcome on the same path — video_validator_compile_timeout (video.ts:172-192) — but the abandoned-compile case that the PR's own test exercises (swift-cache.test.ts:174-197 asserts rejects.toSatisfy(isRequestCanceledError)) carries no reason, hint, or retriable marker. A caller or log that sees the request-canceled error cannot tell "the finalize budget was spent and we deliberately abandoned a cold compile" from an ordinary per-request cancellation, and cannot tell the user to run record stop again the way the compile-timeout hint does. The deadline path avoids this only because the local expired timer usually rejects the race first; the cancellation error is what the shared compile itself settles with. Worth an explicit reason at the abort site so the two budget outcomes are distinguishable.
Suggested change
At the abort site in waitForValidatorCompile (and/or in ensureSwiftExecutable's canceled path), reject with a structured reason (e.g. reason: 'video_validator_compile_abandoned', retriable: true, the same 'Run record stop again' hint) instead of letting the generic request-canceled error reach the caller, so the abandoned-compile outcome is distinguishable from an ordinary request cancellation.
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.
…ize budget `record stop` compiles the AVFoundation video validator on a cold Swift cache under a fixed 30 s bound. On a loaded CI runner that compile takes 17 s to over 30 s, so the stop failed with "xcrun timed out after 30000ms" while most of its 70 s finalization budget was unused. #4065 moved the recording step into the iOS smoke lane, which made this reachable on every PR. The finalizer now hands its budget to the playability checks, and a validator compile with a deadline runs until that deadline. Callers without a deadline keep the 30 s bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s caller's budget The in-flight validator compile took its timeout from the first caller, so a concurrent `record stop` could inherit the 30 s bound. The compile is now shared with only the Swift cache's own bound, and each caller races it against its own budget, failing with reason `video_validator_compile_timeout`. The AVFoundation run after the compile is clamped to the caller's remaining deadline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iting A shared compile that outlived its timed-out caller had no owner: a daemon that exited while it ran left `swiftc` reparented to launchd, past its runCmd timeout, and left the compile's temp output in the Swift cache. The last caller to stop waiting now aborts an unfinished compile, and the Swift cache passes that signal to `swiftc` and to its lock wait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a 1 ms validator run Once the deadline had passed, the validator got a 1 ms timeout, so a clean budget expiry became a thrown "timed out after 1ms". `isPlayableVideo` now reads a deadline that passes before the validator runs as not playable, the outcome `waitForPlayableVideo` already gives when it runs out of time. `waitForPlayableVideo` resolves whether the file played, and the finalizer decides on that verdict, so a file confirmed just before the budget ends is not refused by a second check. The compile-timeout error names its retry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…regression The stub refused a second check only when handed the spent budget, so the base form of that check, which passed no deadline, would have read playable and passed. The stub now refuses any check, and the test asserts none ran. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…able A validator run clamped to the caller's remaining budget now reads as not playable when it times out, instead of throwing an exec timeout. The overlay decides on the verdict waitForPlayableVideo returns, and the wait checks once per attempt with no extra pass after the last one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s end too swift-driver runs each swift-frontend job in its own process group. A SIGKILL or SIGTERM on a canceled validator compile left the frontend compiling with no parent; a live cold-cache abandon showed it. On SIGINT the driver stops its jobs, and SIGKILL still follows after a 2 s grace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erdict Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A validator run that hit its own 10 s bound threw, while one the deadline cut short read as not playable. Both are the same verdict: the file did not prove playable in time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cdf9516 to
a1af133
Compare
There was a problem hiding this comment.
Apex Sentinel: 4 findings moved to a new line on a1af133. The verdict and the full list are in the summary comment.
| ); | ||
| // A budget with no time left reads as not playable; `runCmd` would read a timeout of 0 as none. | ||
| if (runTimeoutMs <= 0) return false; | ||
| const result = await runCmd(validatorPath, [filePath], { |
There was a problem hiding this comment.
🆕 🟡 Minor · Validator-run timeout now returns false before the isSwiftVideoValidatorUnavailable fallback that used to see its stderr
isPlayableVideo used to let a runCmd timeout fall into the catch at the bottom, where isSwiftVideoValidatorError(error) inspects error.details.stderr/stdout (exactly the fields createTimeoutError in packages/host-kit/src/internal/exec.ts:662-669 fills from the killed child) and returns true when they say xcrun/swiftc is missing. That fail-open exists because a host with no working Swift toolchain must not have its recording refused. The new .catch maps any isCommandTimeoutError to undefined inside the try, so if (!result) return false answers before that fallback ever runs. Reproduce: a host whose xcrun stalls and dies printing xcrun: error: unable to find utility "swiftc" — the 10 s (or clamped runTimeoutMs) kill produces a timeout error carrying that stderr, and the verdict flips from true (predecessor) to false, so finalizeScreenRecording now throws recording was not finalized into a playable video instead of accepting the file. Intended per the PR description for the generic case, but the toolchain-missing classification is lost with it. Either re-check the timeout error with isSwiftVideoValidatorError before returning false, or note that a killed validator on a toolchain-less host now reads as unplayable.
| const result = await runCmd(validatorPath, [filePath], { | |
| }).catch((error: unknown) => { | |
| if (isCommandTimeoutError(error)) { | |
| return isSwiftVideoValidatorError(error) ? { exitCode: 0, stdout: '', stderr: '' } : undefined; | |
| } | |
| throw error; | |
| }); | |
| if (!result) return false; |
Details
persona: 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.
| let compiled = videoValidatorExecutablePaths.get(cacheRoot); | ||
| if (!compiled) { | ||
| compiled = compileSwiftSourceText({ | ||
| function joinValidatorCompile(host: HostEnvironment, cacheRoot: string): SharedValidatorCompile { |
There was a problem hiding this comment.
🆕 🟡 Minor · waitForValidatorCompile cancels a compile it started without incrementing waiters first
joinValidatorCompile publishes the map entry and starts compileSwiftSourceText before waitForValidatorCompile executes shared.waiters += 1 (video.ts:153-169 vs 138-152). Today the two run in the same synchronous block so no window exists, but the invariant "a registered compile always has at least one waiter" is invisible and unenforced: the moment the create/subscribe step is separated from the increment (an await introduced between them, or joinValidatorCompile reused by a caller that subscribes later), the last-waiter cleanup at video.ts:197-202 can abort a compile that has a live subscriber and delete the map entry under it, killing a swiftc nobody asked to cancel. joinValidatorCompile also takes a host parameter it never reads, which invites exactly that reuse. Initialize waiters: 1 at creation (and let the joining caller own the increment) or increment before videoValidatorCompiles.set.
Suggested change
Make the waiter count non-zero for the whole lifetime of a registered compile (start waiters at 1 and have the first waitForValidatorCompile release it in its finally), so the cancel-on-last-waiter rule cannot fire before a waiter is registered.
Details
persona: 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.
|
|
||
| const SWIFT_CACHE_VERSION = '2'; | ||
| const LOCK_RETRY_DELAY_MS = 25; | ||
| // swift-driver runs each frontend job in its own process group. It stops them on SIGINT, but |
There was a problem hiding this comment.
🆕 🔵 Nit · Comment on kill describes only the timeout path, but SWIFTC_STOP now also serves request cancellation
ExecOptions.kill is documented as how 'a deadline or a cancelled request stops the child' (packages/host-kit/src/internal/exec.ts:32-38), and this PR is the first use of the field for a cancellation: SWIFTC_STOP is passed unconditionally with signal: params.signal (swift-cache.ts:201-205). The new comment above the constant explains only the swift-driver process-group reason for SIGINT and never says that SIGINT is load-bearing for the abort path too, so a reader who later passes a request-scoped signal into compileSwiftSourceFile/compileSwiftSourceText (which compileSwiftSourceFile cannot do today) would keep the default SIGKILL and re-create the orphaned swift-frontend processes this PR set out to avoid.
Suggested change
Extend the comment to state that the option applies to both the compile deadline and a caller's cancel signal, so any caller that forwards a signal keeps SIGINT.
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.
| settled: boolean; | ||
| }; | ||
|
|
||
| // A compiled helper is valid for the Swift cache it was built into, so each cache root keeps its own. |
There was a problem hiding this comment.
🔵 Nit · Stale doc comment above videoValidatorCompiles still describes the removed timeout behavior
The comment left in place above the new videoValidatorCompiles map ("A compiled helper is valid for the Swift cache it was built into, so each cache root keeps its own.") is directly contradicted by the two new lines added under it and by joinValidatorCompile/waitForValidatorCompile, which drop the entry when a compile fails or when the last waiter gives up — a cache root no longer keeps one compile for its lifetime. The comment also still doesn't say what this map is for now (single-flight compile sharing plus wait-counting), which is the non-obvious part of the new structure.
Suggested change
Rewrite the comment above videoValidatorCompiles to state the current contract: one in-flight compile per Swift cache root, shared by callers, dropped when it fails or when the last waiter stops waiting; a published executable is still reused from disk.
Details
persona: 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.
|
The code looks good at a1af133, and the earlier findings are fixed: the Not blocking, and you can take or leave it: a timed-out validator run now reads as not playable for every caller, so the Android chunk pull in The two open inline threads on the recovery hint when the validator is cut by the deadline (#4150 (comment) and #4150 (comment)) still stand, but the hint is correct and the note above covers them. These threads do not apply and you can resolve them: the 30 s compile wait matches main and a failed compile entry is deleted (#4150 (comment)); overlay.ts throws its own detail-less error, so no compile-timeout details are copied (#4150 (comment)); the |
thymikee
added
the
ready-for-human
|
Timing evidence from CI for the
Limits of this evidence: this PR's three Apple runs compiled in 19.2, 14.4 and 12.6 s, so CI here has not yet exercised a compile over 30 s. Your live run is the only proof of that path. I read the diff but did not run this PR's tests or a device. |
|
Thanks, this matches what we saw. The compile is slow under runner load, not hung, and it fits in the finalize budget.
|
Summary
Since #4065 moved recording into
smoke:capture-close, the iOS smoke lane failed 3 times atrecord stopwithxcrun timed out after 30000ms. It isswiftcdoing a cold compile of the AVFoundation video validator, under a fixed 30 s bound.record stopalready has a 70 s finalization budget meant to cover this compile (#3697). The finalizer now hands it to the playability checks. Callers share one compile, each waits within its own budget, and the last to leave cancels it with SIGINT, so swift-driver also stops its frontend jobs (SIGKILL left them running). A spent budget, or a validator run that times out, reads as not playable. The overlay decides on the verdict the wait returns. 9 files.No-deadline callers keep the fixed 30 s compile wait: Android
pullPlayable(platform-runtime-screen-recording-android-host.ts) and the live-device assertions (test/integration/live-device-e2e/assertions.ts) callisPlayableVideowithout a deadline.Validation
pnpm check:affected --run --head a1af133177(rebased on main): passed,exactCommit=true. Earlier unrelated flakes (stale-ref-warning, Revyl strayGET /, fixed on main by refactor(providers): share one lease table across cloud providers #4158) passed on rerun.cdf951620a,record stoppassed with a 66.5 s compile (stop 73.2 s).video_validator_compile_timeout. No swiftc/swift-frontend,.lockor temp dir was left, and the next stop compiled and passed.🤖 Generated with Claude Code